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

Preserve module identity for symlinks #4761

Merged
merged 4 commits into from Oct 25, 2017

Conversation

Projects
None yet
6 participants
@gaearon
Member

gaearon commented Oct 25, 2017

Summary

We bumped into a problem trying to use Yarn Workspaces with Jest in the React repo.
It is the same issue as described in #3830.

In our case, we had packages/* symlinked through node_modules.

We noticed that importing the same file with the path root/packages/react/module from a test in root/packages/react/folder/__tests__ produced a different instantiation depending on how it is imported:

  • Through the react/folder/module symlink.
  • Or by the relative ../module path.

This is because the old resolving mechanism established resolved paths as:

  • react/folder/module => repo/node_modules/react/folder/module.js
  • ../module => repo/packages/react/folder/module.js

So Jest treated them as two different modules. This created very subtle and hard-to-debug errors (such as duplicate caches in arbitrary module branches depending on the initial imports that "forked" them).

With this fix, the resolving mechanism resolves both cases to repo/packages/react/folder/module.js.

It also fixes a different problem we experienced with Jest/Yarn together not compiling modules.

Test plan

Tests pass in React repo with relative paths, whereas they would fail before.

The reproducing case in https://github.com/tmeasday/jest-symlink-repro used to print twice:

screen shot 2017-10-25 at 4 43 41 pm

Now it prints once, as expected in its README:

screen shot 2017-10-25 at 4 43 43 pm

I added a new integration test and verified it fails on master.

I also added some logging to the test runner for this integration test because when it failed, there used to be no indication at all as to the reasons why it was failing. Removed per feedback.

Locally, I see a test failure in packages/jest-worker/src/__tests__/child.test.js but it also happens in master so is unrelated.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 25, 2017

Member

Apparently the build on master is failing 😞

Should I send another PR to disable the failing test in the meantime?
Since then we don't know if this one works or not.

Member

gaearon commented Oct 25, 2017

Apparently the build on master is failing 😞

Should I send another PR to disable the failing test in the meantime?
Since then we don't know if this one works or not.

@thymikee

This comment has been minimized.

Show comment
Hide comment
@thymikee

thymikee Oct 25, 2017

Collaborator

@gaearon if it's jest-worker test then no need to disable it, because it won't fail on CI (which is run in band). It's a known problem we have.

Collaborator

thymikee commented Oct 25, 2017

@gaearon if it's jest-worker test then no need to disable it, because it won't fail on CI (which is run in band). It's a known problem we have.

if (result) {
// Dereference symlinks to ensure we don't create a separate
// module instance depending on how it was referenced.
result = fs.realpathSync(result);

This comment has been minimized.

@thymikee

thymikee Oct 25, 2017

Collaborator

So basically we're hitting fs one more time, wonder how it affects perfomance.

@thymikee

thymikee Oct 25, 2017

Collaborator

So basically we're hitting fs one more time, wonder how it affects perfomance.

This comment has been minimized.

@gaearon

gaearon Oct 25, 2017

Member

I know it was slow (nodejs/node#2680) and was optimized in nodejs/node@b488b19.

Perhaps counter-intuitively, running on React codebase (with warm caches) makes this PR slightly faster. Maybe there's some indirection in accessing the symlinks later that resolving them removes?

before the change (5 tries)
44.67 real       123.24 user         8.76 sys
44.61 real       123.07 user         8.76 sys
44.58 real       123.44 user         8.77 sys
44.62 real       122.45 user         8.72 sys
46.73 real       124.52 user         8.93 sys

after the change (5 tries)
44.08 real       121.83 user         8.14 sys
43.63 real       121.55 user         8.15 sys
44.01 real       121.95 user         8.23 sys
43.85 real       122.70 user         8.29 sys
44.33 real       122.54 user         8.16 sys

before the change again (5 tries)
44.43 real       122.47 user         8.66 sys
45.18 real       123.94 user         8.91 sys
44.95 real       123.20 user         8.84 sys
44.34 real       123.15 user         8.76 sys
44.50 real       122.88 user         8.72 sys
@gaearon

gaearon Oct 25, 2017

Member

I know it was slow (nodejs/node#2680) and was optimized in nodejs/node@b488b19.

Perhaps counter-intuitively, running on React codebase (with warm caches) makes this PR slightly faster. Maybe there's some indirection in accessing the symlinks later that resolving them removes?

before the change (5 tries)
44.67 real       123.24 user         8.76 sys
44.61 real       123.07 user         8.76 sys
44.58 real       123.44 user         8.77 sys
44.62 real       122.45 user         8.72 sys
46.73 real       124.52 user         8.93 sys

after the change (5 tries)
44.08 real       121.83 user         8.14 sys
43.63 real       121.55 user         8.15 sys
44.01 real       121.95 user         8.23 sys
43.85 real       122.70 user         8.29 sys
44.33 real       122.54 user         8.16 sys

before the change again (5 tries)
44.43 real       122.47 user         8.66 sys
45.18 real       123.94 user         8.91 sys
44.95 real       123.20 user         8.84 sys
44.34 real       123.15 user         8.76 sys
44.50 real       122.88 user         8.72 sys

This comment has been minimized.

@thymikee

thymikee Oct 25, 2017

Collaborator

Thanks for caring about this!

@thymikee

thymikee Oct 25, 2017

Collaborator

Thanks for caring about this!

This comment has been minimized.

@gaearon

gaearon Oct 25, 2017

Member

It could be that it's faster for us (since we use symlinks) but slower on projects that don't use symlinks at all. ¯\(ツ)

@gaearon

gaearon Oct 25, 2017

Member

It could be that it's faster for us (since we use symlinks) but slower on projects that don't use symlinks at all. ¯\(ツ)

@SimenB

This comment has been minimized.

Show comment
Hide comment
@SimenB

SimenB Oct 25, 2017

Collaborator

Failing test on master is tracked in #4745

Collaborator

SimenB commented Oct 25, 2017

Failing test on master is tracked in #4745

@gaearon gaearon referenced this pull request Oct 25, 2017

Closed

Yarn Workspaces Follow-up #11275

13 of 19 tasks complete
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 25, 2017

Member

I also verified that it fixes a different (but related) Yarn/Jest integration problem for us.

After migrating to Yarn Workspaces, our packages was no longer being compiled because we imported them through symlinks. But with this fix, our Babel transform picks them up.

@cpojer told me he’s aware this was an issue. @BYK said GraphQL team also bumped into this.

In my testing, this PR solves it.

Member

gaearon commented Oct 25, 2017

I also verified that it fixes a different (but related) Yarn/Jest integration problem for us.

After migrating to Yarn Workspaces, our packages was no longer being compiled because we imported them through symlinks. But with this fix, our Babel transform picks them up.

@cpojer told me he’s aware this was an issue. @BYK said GraphQL team also bumped into this.

In my testing, this PR solves it.

@cpojer cpojer merged commit 2890498 into facebook:master Oct 25, 2017

6 of 7 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
ci/circleci: test-and-deploy-website Your tests passed on CircleCI!
Details
ci/circleci: test-browser Your tests passed on CircleCI!
Details
ci/circleci: test-node-4 Your tests passed on CircleCI!
Details
ci/circleci: test-node-6 Your tests passed on CircleCI!
Details
ci/circleci: test-node-8 Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cpojer

This comment has been minimized.

Show comment
Hide comment
@cpojer

cpojer Oct 25, 2017

Contributor

Thanks so much @gaearon for making this happen. This definitely seems like the right fix for this issue.

I published 21.3.0-beta.3 so you can start using this fix.

Contributor

cpojer commented Oct 25, 2017

Thanks so much @gaearon for making this happen. This definitely seems like the right fix for this issue.

I published 21.3.0-beta.3 so you can start using this fix.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 25, 2017

Member

Sweet. Thanks. 💃

Member

gaearon commented Oct 25, 2017

Sweet. Thanks. 💃

@gaearon gaearon deleted the gaearon:fix-symlinks branch Oct 26, 2017

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Oct 26, 2017

Member

Unrelated, but I noticed that in 21.3.0-beta.3 even caught errors in tests like

    fit('should throw on children for void elements', () => {
      const container = document.createElement('div');
      let caughtErr;
      try {
        ReactDOM.render(<input>children</input>, container); // throws
      } catch (err) {
        caughtErr = err;
      }
      expect(caughtErr).not.toBe(undefined);
    });

end up being reported as failures. This isn't the case in 21.2.1.

I know I could use toThrow but that doesn't let me transform message (need to massage it) before asserting.

Is this intentional or a bug? Worth making an issue?

Member

gaearon commented Oct 26, 2017

Unrelated, but I noticed that in 21.3.0-beta.3 even caught errors in tests like

    fit('should throw on children for void elements', () => {
      const container = document.createElement('div');
      let caughtErr;
      try {
        ReactDOM.render(<input>children</input>, container); // throws
      } catch (err) {
        caughtErr = err;
      }
      expect(caughtErr).not.toBe(undefined);
    });

end up being reported as failures. This isn't the case in 21.2.1.

I know I could use toThrow but that doesn't let me transform message (need to massage it) before asserting.

Is this intentional or a bug? Worth making an issue?

@milesj

This comment has been minimized.

Show comment
Hide comment
@milesj

milesj Oct 26, 2017

Thanks so much for this. I've been plagued by it since workspaces was released!

milesj commented Oct 26, 2017

Thanks so much for this. I've been plagued by it since workspaces was released!

gaearon added a commit to gaearon/react that referenced this pull request Oct 26, 2017

gaearon added a commit to facebook/react that referenced this pull request Oct 26, 2017

Update Jest and remove hacks (#11372)
* Update Jest

* Remove hacks for Jest + Workspace integration

They were fixed by facebook/jest#4761.

* Use relative requires in tests relying on private APIs

I changed them to absolute to work around a Jest bug.
The bug has been fixed so I can revert my past changes now.

Ethan-Arrowood added a commit to Ethan-Arrowood/react that referenced this pull request Dec 8, 2017

Update Jest and remove hacks (facebook#11372)
* Update Jest

* Remove hacks for Jest + Workspace integration

They were fixed by facebook/jest#4761.

* Use relative requires in tests relying on private APIs

I changed them to absolute to work around a Jest bug.
The bug has been fixed so I can revert my past changes now.

davwards added a commit to davwards/elementals that referenced this pull request Dec 9, 2017

Extract gherkin-runner to separate module
Discovered while doing this some weirdness involving babel
transpilation. gherkin-runner wasn't getting transpiled when tests run.

Found some discussion of the issue, including an indication that a fix
is due out in jest soon:

facebook/jest#4761

I updated jest to 21.3.0-beta.3 to see if that helped. It didn't at
first, which turned out to be because it wasn't under the original
package directory, so babel didn't know what transforms to run (?).
(These thoughts are based on discussion here:)

babel/babel-loader#293

I added some babel configuration to the root-level package.json which
fixed the issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment