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.

Sign up for GitHub

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
Merged

Preserve module identity for symlinks #4761

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
836f8fd
Preserve module identity for symlinks
gaearon Oct 25, 2017
a1c65e9
Add changelog entry
gaearon Oct 25, 2017
8f651e7
Remove unintentional whitespace change
gaearon Oct 25, 2017
50f000e
Revert integration test to undebuggable state
gaearon Oct 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
* `[jest-message-util]` Always remove node internals from stacktraces ([#4695](https://github.com/facebook/jest/pull/4695))
* `[jest-resolve]` changes method of determining builtin modules to include missing builtins ([#4740](https://github.com/facebook/jest/pull/4740))
* `[pretty-format]` Prevent error in pretty-format for window in jsdom test env ([#4750](https://github.com/facebook/jest/pull/4750))
* `[jest-resolve]` Preserve module identity for symlinks ([#4761](https://github.com/facebook/jest/pull/4761))

### Features
* `[jest-environment-*]` [**BREAKING**] Add Async Test Environment APIs, dispose is now teardown ([#4506](https://github.com/facebook/jest/pull/4506))
Expand Down
6 changes: 5 additions & 1 deletion integration_tests/__tests__/resolve.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,9 @@ const runJest = require('../runJest');

test('resolve platform modules', () => {
const result = runJest('resolve');
expect(result.status).toBe(0);
const didFail = result.status !== 0;
if (didFail) {
console.log(result.stderr);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a leftover, right? I know these integration tests don't give great error feedback, but it gets better while running inside specific directory (when debugging what's wrong)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was not obvious at all. Contributing document doesn't mention this :-(
I spent half an hour guessing why the test doesn't work without feedback.

As I wrote above:

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.

I can remove though.

Copy link
Member

Choose a reason for hiding this comment

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

Can you open up a separate issue for this? If nothing else it should be documented, but maybe we can make it better

expect(didFail).toBe(false);
});
6 changes: 6 additions & 0 deletions integration_tests/resolve/__tests__/resolve.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,3 +56,9 @@ test('should resolve filename.json', () => {
expect(testRequire('../test4')).not.toThrow();
expect(platform.extension).toBe('json');
});

test('should preserve identity for symlinks', () => {
expect(require('../../../packages/jest-resolve')).toBe(
require('jest-resolve')
);
});
5 changes: 5 additions & 0 deletions packages/jest-resolve/src/default_resolver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,11 @@ function resolveSync(target: Path, options: ResolverOptions): Path {
if (isDirectory(dir)) {
result = resolveAsFile(name) || resolveAsDirectory(name);
}
if (result) {
// Dereference symlinks to ensure we don't create a separate
// module instance depending on how it was referenced.
result = fs.realpathSync(result);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for caring about this!

Copy link
Contributor Author

@gaearon gaearon Oct 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

}
return result;
}

Expand Down