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 module identity preservation with symlinks and browser field resolution #9511

Merged
merged 6 commits into from
Feb 4, 2020

Conversation

rtsao
Copy link
Contributor

@rtsao rtsao commented Feb 3, 2020

Summary

Fixes #9510 Fixes #7840

Test plan

I've added an e2e test with the same setup as the minimal reproduction in https://github.com/rtsao/jest-bug-repro-preserve-symlinked-module-ids

@rtsao rtsao changed the title Fix module id preservation with symlinks when using browser field Fix module identity preservation with symlinks and browser field resolution Feb 3, 2020
@rtsao
Copy link
Contributor Author

rtsao commented Feb 3, 2020

I see a few ways of fixing this issue:

  1. The current implementation, which simply duplicates the logic added in Preserve module identity for symlinks #4761:

https://github.com/facebook/jest/blob/964dc1619b8c5345a648ef79bcd87c087995255a/packages/jest-resolve/src/defaultResolver.ts#L101-L105

  1. Fix this upstream Resolve symlinked modules correctly browserify/browser-resolve#92, although it sounds like this package is not being actively maintained.

  2. Extract out the critical logic from browser-resolve (i.e. the parsing of the "browser" field and mapping files) and integrate it into the normal resolution code. This might be preferable to the current situation where the core resolution code is completely different from the browser field resolution code. Instead, the core resolution code should probably be shared, with some extra code for optionally handling the browser field.

I'm happy to do any of these.

@SimenB
Copy link
Member

SimenB commented Feb 4, 2020

Hey @rtsao, thanks for the PR! Of the 3 options you outline, 3 sounds the best, especially as (like you note) upstream seems somewhat unmaintained. It does impose a maintenance burden though.

I opened up #9505 yesterday, where I bring up using the resolve package again - I wonder if that could be enhanced to also look at the browser field - especially as the module lives in the browserify org

@rtsao
Copy link
Contributor Author

rtsao commented Feb 4, 2020

@SimenB Do you have thoughts on the best way to set up an e2e test for this? Perhaps a script that generates the symlinked packages in fixtures/?

@SimenB
Copy link
Member

SimenB commented Feb 4, 2020

@rtsao take a look at the test added in #9351, it includes a util for e2e test with symlink

@codecov-io
Copy link

codecov-io commented Feb 4, 2020

Codecov Report

Merging #9511 into master will decrease coverage by <.01%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #9511      +/-   ##
=========================================
- Coverage   65.01%     65%   -0.01%     
=========================================
  Files         283     283              
  Lines       12148   12151       +3     
  Branches     3009    3010       +1     
=========================================
+ Hits         7898    7899       +1     
- Misses       3607    3608       +1     
- Partials      643     644       +1
Impacted Files Coverage Δ
packages/jest-resolve/src/defaultResolver.ts 68.49% <60%> (-1.51%) ⬇️

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 964dc16...2fdb2fd. Read the comment docs.

Co-Authored-By: Simen Bekkhus <sbekkhus91@gmail.com>
@SimenB SimenB merged commit ee9e2ea into jestjs:master Feb 4, 2020
@SimenB
Copy link
Member

SimenB commented Feb 4, 2020

Thanks!

@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 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants