Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: validate config with jest-validate #205

Closed
wants to merge 8 commits into from

Conversation

thymikee
Copy link
Contributor

Summary

This PR introduces validation for the new metro-config. For now I'm keen on only validating new config, because I can see there's yet unused convertConfig which, I guess, defeats the purpose. Let me know otherwise.

This is still work in progress, as I need to fix some stuff upstream (e.g. recursive object validation and pretty-printing). Just letting you know I'm working on it :)

Test plan

Added a test.

cc @CompuIves @cpojer

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 26, 2018
@codecov-io
Copy link

Codecov Report

Merging #205 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #205      +/-   ##
==========================================
+ Coverage   84.57%   84.59%   +0.01%     
==========================================
  Files         133      134       +1     
  Lines        4377     4381       +4     
  Branches      681      681              
==========================================
+ Hits         3702     3706       +4     
  Misses        598      598              
  Partials       77       77
Impacted Files Coverage Δ
packages/metro-config/src/loadConfig.js 75.86% <100%> (+1.31%) ⬆️
packages/metro-config/src/defaults/validConfig.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5370ff...5c6c9c5. Read the comment docs.

@CompuIves
Copy link
Contributor

This is so cool @thymikee, thanks for building this!

For now I'm keen on only validating new config, because I can see there's yet unused convertConfig which, I guess, defeats the purpose. Let me know otherwise.

I agree, we only have convertConfig available right now to make the transition from the old config easier, but we're planning to remove this anyways in a near-future version.

@thymikee
Copy link
Contributor Author

thymikee commented Aug 3, 2018

Update: jestjs/jest#6802

@thymikee
Copy link
Contributor Author

thymikee commented Aug 9, 2018

jestjs/jest#6802 was merged, so now we wait for new Jest version to be released by @mjesun :)

@thymikee thymikee changed the title [WIP] validate config with jest-validate feat: validate config with jest-validate Oct 24, 2018
@cpojer
Copy link
Contributor

cpojer commented Feb 25, 2019

@thymikee can you rebase this PR with the new Jest version? I'll land it then :)

…ation

* upstream/master: (42 commits)
  Bump [email protected]
  Fix entryFile always is assumed to end with `.js` extension (facebook#310)
  Adds support for `publicPath` to enable serving assets from different locations. (facebook#299)
  Make dynamic import interoperable for CJS and ESM
  Add custom serializer (facebook#309)
  React sync for revisions 4773fdf...3ff2c7c
  metro-buck: disable inline IDs in dev mode
  Simplify helper function
  Rewrite traverseDependency-integration-tests to make them module resolution unit tests
  RN: Revert React 16.6 Sync
  React sync for revisions 4773fdf...bf9fadf
  Bump react-deep-force-update
  Use getFileIterator() from jest-haste-map
  Deploy Flow v0.85 to xplat/js
  metro-buck: add explicit import() prefetch feature
  jest: remove jasmine usages
  jest: upgrade to 24.0.0-alpha.4
  Fix source map loading in the remote debugger
  Fix metro visualizer transformation
  Update babel to version 7
  ...
…ation

* upstream/master: (122 commits)
  Bump Metro to v0.53.0
  Improve flow coverage in Metro
  Enable platform transforms for dev too, but keep it safe for MetroResolver
  Use function maps to symbolicate traces
  Move react-native-symbolicate to metro repo
  Migrated Metro from babel-jest to a custom transformer
  React sync for revisions f24a0da...8e25ed2
  Revert D14168466: Perform security fixes
  Adds a missing dependency to metro
  Bumps antd to fix missing peerdeps
  Updates the package.json to match the Yarn output
  Improve prelude code
  Perform security fixes
  metro-buck: setup __DEV__ and other variables on the global object
  Add README and development script (facebook#292)
  Allow custom BABEL_ENV (facebook#364)
  Update siteConfig.js (facebook#327)
  docs: mention lerna in CONTRIBUTING.md (facebook#338)
  Revert D14024934: metro: keep track of significant dependencies ordering
  metro-buck: Hermes: no args to async wrapper if no prefetch
  ...
@thymikee
Copy link
Contributor Author

thymikee commented Mar 3, 2019

Looks like I'll need to sort out printing functions in jest-validate, toString() is not the best idea :D

@cpojer
Copy link
Contributor

cpojer commented Mar 4, 2019

Let me know once that part is ready so we can move ahead on this PR :)

@cpojer
Copy link
Contributor

cpojer commented Mar 15, 2019

ping @thymikee let's make it happen? :)

…ation

* upstream/master:
  Upgrade jest to 24.5.0
  Bump Prettier to 1.16.4
  Bump Metro to v0.53.1
  Another pass at Metro Flow annotation coverage
  Add CLI option to ignore function map
  Default to lazy requires for all public RN exports (facebook#362)
  FileStore now recreates parent cache folders if a write fails (facebook#370)
  Support symbolicating file:line:column stack frames
  Add connection verification
  Improve Flow annotation coverage in Metro
  Drop leading newline
  Deploy 0.94 to xplat
  Add new metro flag for running inspector proxy
  Perform security fixes
@thymikee
Copy link
Contributor Author

Changed the test to avoid printing functions as it's Jest's responsibility to test the output (which also may slightly change at any time, so the test would be prone to errors). Regarding jest-validate I think it may still be nice to see function signature in validation message as it usually doesn't go through istanbul. Of course it may be altered by Babel, but at least the number of arguments will be shown.

The CI failures seem unrelated.

Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you :)

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@cpojer
Copy link
Contributor

cpojer commented Mar 18, 2019

Seems to fail at the import step, no idea why. Retrying.

@thymikee
Copy link
Contributor Author

Can you share the error?

@cpojer
Copy link
Contributor

cpojer commented Mar 18, 2019

Nothing from your side, I've been fixing this up for the last hour.

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Mar 18, 2019
Summary:
**Summary**

This PR introduces validation for the new `metro-config`. For now I'm keen on only validating new config, because I can see there's yet unused `convertConfig` which, I guess, defeats the purpose. Let me know otherwise.

This is still work in progress, as I need to fix some stuff upstream (e.g. recursive object validation and pretty-printing). Just letting you know I'm working on it :)

**Test plan**

Added a test.

cc CompuIves cpojer
Pull Request resolved: facebook/metro#205

Differential Revision: D14501222

Pulled By: cpojer

fbshipit-source-id: 0ab8f3ad14d6f33690d5f57fd1e7487f3a4a8c71
@facebook-github-bot
Copy link
Contributor

@cpojer merged this pull request in b040d8a.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants