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

fix dependency name clash bug #6687

Merged
merged 2 commits into from Jul 17, 2018

Conversation

aaronabramov
Copy link
Contributor

@aaronabramov aaronabramov commented Jul 12, 2018

to fix the bug i moved prettier and babel-traverse outside of node VM, so it is only required once per worker (and not in each individual test).

by doing this we'll also avoid taking the source code of our these dependencies through our resolution/transformation pipeline, which solves the issue of conflicting module names (since babel is no longer required from within the test code any more)

old description:

It seems like a pretty serious bug, i don't have a fix yet, but i'm submitting the repro in case anyone >knows what's the best way to approach it.

TL;DR:

  • we use @providesModule system at fb that let's us require files directly by their names (e.g. >require('myModule111') that'll resolve to some/dir/myModule111.js)
  • npm can import dependencies that have similar names
  • we have a module invariant.js in fb codebase
  • inline-snapshots introduced a babel dependency that also depends on invariant pakcage from >npm (https://github.com/babel/babel/blob/c7980b2b90744098ed5d67a105f8f6e7270e7cbe/packages/babel-traverse/src/path/index.js#L5)
  • when we run the test, babel-traverse requires invariant that resolves to the local invarian.js >and everything breaks because our invariant.js uses flow types that aren't parseable.

@aaronabramov
Copy link
Contributor Author

most likely i can fix it by moving require('jest-snapshot') out of the VM and require it in the outer level, but that means we'll have the same problem sometime in the future :)

@SimenB
Copy link
Member

SimenB commented Jul 13, 2018

In general it would be awesome if we could require everything jest needs outside of the VM anyways, isn't it?
All our requires are lazy, would it still work to hoist it up?

@aaronabramov
Copy link
Contributor Author

@SimenB yeah, ideally we should do this. in this case there's a global state that snapshots have (serializers and might be more) and it has to be required from within the VM. There shouldn't be anything that prevents us from changing that and just passing an instance of the state down to the VM

@aaronabramov aaronabramov force-pushed the module_name_clash branch 3 times, most recently from 06dbac2 to e621813 Compare July 16, 2018 22:24
@aaronabramov aaronabramov changed the title dependency name clash fix dependency name clash bug Jul 16, 2018
@aaronabramov
Copy link
Contributor Author

added the fix

@SimenB
Copy link
Member

SimenB commented Jul 17, 2018

New test fails on appveyor, not really sure why... It's complaining about invalid json in babelrc.

This should also have a changelog entry

@codecov-io
Copy link

Codecov Report

Merging #6687 into master will decrease coverage by 0.15%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6687      +/-   ##
==========================================
- Coverage   63.71%   63.55%   -0.16%     
==========================================
  Files         235      235              
  Lines        8998     8997       -1     
  Branches        4        4              
==========================================
- Hits         5733     5718      -15     
- Misses       3264     3278      +14     
  Partials        1        1
Impacted Files Coverage Δ
.../src/legacy_code_todo_rewrite/jest_adapter_init.js 0% <ø> (ø) ⬆️
packages/jest-snapshot/src/State.js 0% <0%> (ø) ⬆️
...ircus/src/legacy_code_todo_rewrite/jest_adapter.js 0% <0%> (ø) ⬆️
packages/jest-jasmine2/src/setup_jest_globals.js 0% <0%> (ø) ⬆️
packages/jest-snapshot/src/inline_snapshots.js 85.96% <100%> (+0.25%) ⬆️
packages/jest-cli/src/lib/update_global_config.js 72.72% <0%> (-22.73%) ⬇️
packages/jest-haste-map/src/crawlers/watchman.js 94.44% <0%> (-0.43%) ⬇️

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 ee0f92e...56296b8. Read the comment docs.

@aaronabramov aaronabramov merged commit f45d1c9 into jestjs:master Jul 17, 2018
@aaronabramov aaronabramov deleted the module_name_clash branch July 17, 2018 21:46
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants