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

Eagerly evaluate inline requires in Jest #7245

Merged
merged 2 commits into from
Jul 16, 2016
Merged

Eagerly evaluate inline requires in Jest #7245

merged 2 commits into from
Jul 16, 2016

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Jul 11, 2016

I inlined some requires in #7188 to fix the build size regression.
However this caused an issue with Jest due to it resetting module registry between tests.

This is a temporary fix to #7240.
It should be reverted as part of #7178.

Test plan:

  1. Verify "Missing React element for debugID" warning when testing components with Jest #7240 is fixed by following its repro instructions. You would need to build React and copy build/modules/* into its node_modules/react/lib/.
  2. Verify a React example (e.g. basic-click-counter) in the repo runs with minified and unminified React.
  3. Check that the minified build size has not increased.

Reviewers: @zpao @keyanzhang

(Yes I know it’s super ugly. The upside is I know Jest a little better now!)

I inlined some requires in #7188 to fix the build size regression.
However this caused an issue with Jest due to it resetting module registry between tests.

This is a temporary fix to #7240.
It should be reverted as part of #7178.
@gaearon
Copy link
Collaborator Author

gaearon commented Jul 11, 2016

@cpojer Does this look reasonable to you as a stopgap solution? (We have a better solution in #7178.)

@gaearon
Copy link
Collaborator Author

gaearon commented Jul 11, 2016

@keyanzhang Do you think we’ll get #7178 in this week? If yes, maybe we don’t need to bother with this fix.

@keyz
Copy link
Contributor

keyz commented Jul 11, 2016

@gaearon I'll get that PR ready in a few days. Just to clarify -- we run these tests with the development build right?

@cpojer
Copy link
Contributor

cpojer commented Jul 12, 2016

Yeah this seems reasonable. I'm sorry for the pain.

I'm however unsure how rollup would solve this as people will use the dev code (react/lib/*), not the production build when writing unit tests.

@gaearon
Copy link
Collaborator Author

gaearon commented Jul 12, 2016

Yes, we run tests with development CommonJS build. Rollup would solve this because we can move requires back to the top of the file (like Jest prefers) but it would still eliminate dead code in prod. Browserify is not that smart which is why I inlined them, but I won't have to after switching to Rollup.

@cpojer
Copy link
Contributor

cpojer commented Jul 12, 2016

oh, yeah, that makes sense!

@ghost ghost added the CLA Signed label Jul 12, 2016
@@ -17,6 +17,17 @@ var ReactPropTypesSecret = require('ReactPropTypesSecret');
var invariant = require('invariant');
var warning = require('warning');

var ReactComponentTreeDevtool;

if (process.env.NODE_ENV === 'test') {
Copy link
Member

Choose a reason for hiding this comment

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

I fully expect this to break internally unless we check if (process && process.env && process.env.NODE_ENV = 'test').

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Aah, __DEV__ “works” because it’s how it’s called internally! :steve_jobs_fireworks:

@gaearon
Copy link
Collaborator Author

gaearon commented Jul 14, 2016

Okay, this should work, right? This is getting super hacky. 😢

@gaearon
Copy link
Collaborator Author

gaearon commented Jul 14, 2016

  1. This should work in FB because typeof process should give undefined as it’s an unknown global.
  2. This should work in CommonJS because process should give whatever your bundler gives you, and we don’t care whether it exists or not thanks to the same check.
  3. This should work in our UMD builds because we don’t use process anywhere except those "test" checks so it’s safe to force Browserify to use undefined there. Our __DEV__ clauses still work because they get envified.
  4. This works in Jest because, well, process exists there.

@ghost ghost added the CLA Signed label Jul 14, 2016
@gaearon
Copy link
Collaborator Author

gaearon commented Jul 15, 2016

@zpao Shall we do it?

@zpao
Copy link
Member

zpao commented Jul 15, 2016

Sure, but let's make sure this doesn't stick around for long.

@ghost ghost added the CLA Signed label Jul 15, 2016
@gaearon gaearon merged commit 15ae585 into facebook:master Jul 16, 2016
@gaearon gaearon deleted the jest-fix branch July 16, 2016 19:51
@zpao zpao modified the milestones: 15.3.0-test, 15-next Jul 21, 2016
@zpao zpao modified the milestones: 15.3.0-test, 15.3.0 Jul 21, 2016
zpao pushed a commit that referenced this pull request Jul 22, 2016
* Eagerly evaluate inline requires in Jest

I inlined some requires in #7188 to fix the build size regression.
However this caused an issue with Jest due to it resetting module registry between tests.

This is a temporary fix to #7240.
It should be reverted as part of #7178.

* Make the hack work in all environments

(cherry picked from commit 15ae585)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants