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

Update getPackageMain logic to retrieve the first entry that exists #1237

Conversation

rkt2spc
Copy link

@rkt2spc rkt2spc commented Apr 24, 2018

Currently, node_modules package entry is resolved in the following order.
pkg.module -> pkg['jsnext:main'] -> pkg.browser -> pkg.main -> ${pkg.pkgdir}/index.js

I keep the same order but will fallback to the first EXISTS entry. That way we can avoid common library author mistakes, which results in a lot of cannot resolve dependency... issues.

const stat = await fs.stat(entry);
if (stat.isDirectory()) return path.resolve(entry, 'index');
return entry;
} catch (e) {
Copy link
Author

Choose a reason for hiding this comment

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

I think we should just blow-up here if an entry record declared in package.json doesn't exists instead of handling it.

@codecov-io
Copy link

Codecov Report

Merging #1237 into master will increase coverage by 0.3%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1237     +/-   ##
=========================================
+ Coverage   88.28%   88.59%   +0.3%     
=========================================
  Files          77       77             
  Lines        4355     4356      +1     
=========================================
+ Hits         3845     3859     +14     
+ Misses        510      497     -13
Impacted Files Coverage Δ
src/Resolver.js 100% <100%> (ø) ⬆️
src/assets/JSONAsset.js 84.61% <0%> (-7.7%) ⬇️
src/assets/VueAsset.js 80% <0%> (-6.07%) ⬇️
src/visitors/dependencies.js 89.13% <0%> (-5.44%) ⬇️
src/visitors/globals.js 92.3% <0%> (-3.85%) ⬇️
src/assets/TypeScriptAsset.js 97.29% <0%> (-2.71%) ⬇️
src/transforms/babel.js 94.02% <0%> (-1.5%) ⬇️
src/assets/GraphqlAsset.js 100% <0%> (ø) ⬆️
src/assets/RustAsset.js 91.66% <0%> (+1.19%) ⬆️
src/assets/SASSAsset.js 93.61% <0%> (+2.12%) ⬆️
... and 7 more

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 bb7f4aa...5ff121c. Read the comment docs.

Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! Could you also add a test?

if (!main || main === '.' || main === './') {
main = 'index';
try {
const stat = await fs.stat(entry);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe instead of doing the actual resolution here (and then duplicating it again later), we could just have getPackageMain return an array of possible paths. Then, loadDirectory could just loop through them and try each one: https://github.com/parcel-bundler/parcel/blob/master/src/Resolver.js#L188-L191

Copy link
Author

Choose a reason for hiding this comment

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

Ok, but I'm a little bit busy right now. I'll come back and take a deeper look into this next month.

@devongovett
Copy link
Member

Closing in favor of #1577.

@devongovett devongovett closed this Jul 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants