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
Update getPackageMain logic to retrieve the first entry that exists #1237
Conversation
const stat = await fs.stat(entry); | ||
if (stat.isDirectory()) return path.resolve(entry, 'index'); | ||
return entry; | ||
} catch (e) { |
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 we should just blow-up here if an entry record declared in package.json doesn't exists instead of handling it.
Codecov Report
@@ 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
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 for working on this! Could you also add a test?
if (!main || main === '.' || main === './') { | ||
main = 'index'; | ||
try { | ||
const stat = await fs.stat(entry); |
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.
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
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.
Ok, but I'm a little bit busy right now. I'll come back and take a deeper look into this next month.
Closing in favor of #1577. |
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.