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

jest-haste-map: throw when trying to get a duplicated module #3976

Merged
merged 3 commits into from
Jul 6, 2017

Conversation

jeanlauliac
Copy link
Contributor

Summary

Right now, if a module is duplicated, we remove it from the module map, so that when we try to resolve it later, it doesn't return one or the other randomly, that would be incorrect, but instead nothing.

I realised this approach is better then returning a random module, but is still incorrect, because the callsite (resolution algorithm) could then try to resolve the module name as a Node.js package, and find some other module. This is just as random, and incorrect.

I believe the correct approach in case of duplicate modules for a single name is to prevent any further resolution by throwing an exception with sufficient metadata for the callsite, or the user, to make a decision. This changeset implements this correct behavior.

Test plan

yarn jest packages/jest-haste-map/src/__tests__/index.test.js

};
export type DuplicatesSet = {[filePath: string]: /* type */ number};
Copy link
Member

Choose a reason for hiding this comment

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

mind moving this up, before it is first used?

@cpojer
Copy link
Member

cpojer commented Jul 6, 2017

Looks solid to me. Nice work. I'm slightly worried that this is going to incur a minor overhead for looking up a module, because of the many more added assertions, but we are paying the price for correctness.

@jeanlauliac
Copy link
Contributor Author

jeanlauliac commented Jul 6, 2017

I'm slightly worried that this is going to incur a minor overhead for looking up a module, because of the many more added assertions, but we are paying the price for correctness.

Totally, I'm concerned about the same. But it's a pretty simple hashmap access + function call, so it should be nicely optimized. I was considering doing the map check first so that if it does resolves, we're done. But in practice I don't think that'd change much, especially as in most case the first two conditions would be passed by anyway (platform is generic for most modules).

@codecov-io
Copy link

codecov-io commented Jul 6, 2017

Codecov Report

Merging #3976 into master will increase coverage by 0.09%.
The diff coverage is 80.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3976      +/-   ##
==========================================
+ Coverage   59.84%   59.93%   +0.09%     
==========================================
  Files         196      196              
  Lines        6758     6787      +29     
  Branches        6        6              
==========================================
+ Hits         4044     4068      +24     
- Misses       2711     2716       +5     
  Partials        3        3
Impacted Files Coverage Δ
packages/jest-haste-map/src/index.js 92.8% <50%> (ø) ⬆️
packages/jest-haste-map/src/module_map.js 80% <82.05%> (+2.22%) ⬆️
packages/jest-matchers/src/matchers.js 99.37% <0%> (-0.01%) ⬇️
packages/jest-matchers/src/index.js 97.46% <0%> (+0.13%) ⬆️

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 05343f4...66dd4f5. Read the comment docs.

@jeanlauliac
Copy link
Contributor Author

Whether this is a breaking change or not is to be discussed. I believe it's not really one, because having duplicate modules is broken anyway, as resolution algos would then try to do a Node.js-style resolve, that could pick up some random, unrelated module.

At the same time, I don't want to cause hard crash on people's project especially if the duplicates are in their transitive dependencies. So I'd suggest considering this a breaking change.

@cpojer cpojer merged commit c11a98e into jestjs:master Jul 6, 2017
@cpojer
Copy link
Member

cpojer commented Jul 6, 2017

It shouldn't really matter for external folks, because haste is not really used outside of FB. Thanks for the fix!

@jeanlauliac
Copy link
Contributor Author

Haste is used for the Windows React Native hack, afaik. But normally they use "windows" platform extensions and there's no module conflicts, that would resolve incorrectly anyway.

tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017
…3976)

* jest-haste-map: throw when trying to get a duplicated module

* fix lint

* move type up as per comment
@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 13, 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.

4 participants