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

get only existing package main #1577

Merged
merged 7 commits into from Jul 21, 2018
Merged

Conversation

ranfdev
Copy link
Contributor

@ranfdev ranfdev commented Jun 18, 2018

Sometimes a package has in his package.json a source field, but the package is already bundled/compiled, so there isn't any source file inside that package. Now, before returning any package file, parcel checks if the file exists. If it doesn't exists, parcel tries to use another file, until a real file is found. fixes #1568

I still need to fix the error...

src/Resolver.js Outdated
for (let source of [pkg.source, pkg.module, browser, pkg.main, 'index']) {
if (
typeof source === 'string' &&
(await fs.exists(path.resolve(pkg.pkgdir, source)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi!

According to node.js docs fs.exists method is deprecated. In order to check if files exists we can use fs.access method or sync version fs.accessSync.

@ranfdev
Copy link
Contributor Author

ranfdev commented Jun 19, 2018

The errors are not related to this pr

src/Resolver.js Outdated
try {
// else, check if the file main exists/is accessible. if it does, return the path of the file.
const resolvedPath = path.resolve(pkg.pkgdir, main);
await fs.access(resolvedPath);
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this the responsibility of the library anyone is using?

Shouldn't main always be accesible? Otherwise it's a bug in package.json from the pkg that's being processed

Copy link
Contributor Author

@ranfdev ranfdev Jun 21, 2018

Choose a reason for hiding this comment

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

// else, check if the file main exists/is accessible

here i'm referring to the main variable in general, not only the pkg.main file. The value of the main variable in that moment could be pkg.source.
pkg.source is not always present in compiled packages, that's why parcel needs to check if it's accessible. If it's not, parcel should try to resolve the next possibleMain (e.g. pkg.module), until it finds an existing one.
Before this pr, parcel would try to load pkg.source even if it isn't accessible

Copy link
Member

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 have getPackageMain return an array of possible paths, and have loadDirectory loop through them and try each one. Since it is already reading files there, we would avoid checking twice.

@DeMoorJasper
Copy link
Member

Could you resolve the conflicts?

@ranfdev
Copy link
Contributor Author

ranfdev commented Jun 25, 2018

Ok

src/Resolver.js Outdated
try {
// else, check if the file main exists/is accessible. if it does, return the path of the file.
const resolvedPath = path.resolve(pkg.pkgdir, main);
await fs.access(resolvedPath);
Copy link
Member

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 have getPackageMain return an array of possible paths, and have loadDirectory loop through them and try each one. Since it is already reading files there, we would avoid checking twice.

@ranfdev
Copy link
Contributor Author

ranfdev commented Jul 12, 2018

@devongovett this should do the job

@devongovett devongovett merged commit f431b78 into parcel-bundler:master Jul 21, 2018
devongovett pushed a commit that referenced this pull request Oct 15, 2018
devongovett pushed a commit that referenced this pull request Oct 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't resolve some dependencies
4 participants