-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Resolve dynamic dependencies correctly when a mapping exists #9303
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9303 +/- ##
==========================================
+ Coverage 64.99% 65.01% +0.01%
==========================================
Files 278 278
Lines 11912 11913 +1
Branches 2934 2935 +1
==========================================
+ Hits 7742 7745 +3
+ Misses 3539 3538 -1
+ Partials 631 630 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, tested against https://github.com/jeysal/jest-7511-repro and looks like it works! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a unit test makes sense here in addition to the e2e test
@SimenB, I'm trying to write a unit test, but for some reason I can't get the unit tests to actually run. Running Am I missing something here in trying to run the tests? |
You should run |
@SimenB + others - I've added the unit test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonderful, thank you!
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. |
Summary
Fix #7511
When resolving a dynamic dependency (e.g.
require(`~/${dynamicName}.js`)
, handle the case where there is a dependency resolution mapping correctly. Do this by catching all errors during the module resolution process, and not adding a new dependency is the would-be dependency cannot be resolved to a real module.Test plan
I added an additional e2e test that reproduces the failing case. (Thanks to @jeysal for their previous attempt to isolate the cause.) I didn't add additional unit tests for this because I felt it would complicate matters, and figured that the e2e test covered the case entirely.