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

Symlink realpaths #1125

Closed
jamiebuilds opened this issue Apr 2, 2018 · 13 comments
Closed

Symlink realpaths #1125

jamiebuilds opened this issue Apr 2, 2018 · 13 comments
Labels
🐛 Bug Stale Inactive issues

Comments

@jamiebuilds
Copy link
Member

Currently, (as of 1.7) if you have a project that looks like this:

File Contents
index.js require('./target');
require('./symlink')
target.js ...
symlink.js (symlink ./target.js)

The contents and dependencies of target.js will be duplicated.

This is because Parcel does not fs.realpath file paths as they are resolved, and for good reason: If you have a npm link-d package in your node_modules and you fs.realpath it before resolving its dependencies, then you can get multiple copies of the same dependencies. Basically, Parcel is following Node's --preserve-symlinks option.

Dependency resolution should continue to work this way, but before we add a module to the graph, we should then fs.realpath it and if it already exists we should not re-add it. This also matches Node's behaviour because symlinked modules do not get re-evaluated in Node, where right now they do (in effect) in Parcel.

We should:

  • Continue resolving dependencies of modules using the non-realpath'd file path.
  • Call fs.realpath on modules before we add them to the graph and avoid them ending up in our bundles multiple times.
@jamiebuilds
Copy link
Member Author

I just noticed something... for some reason target.js doesn't get duplicated when you pass --no-hmr and I'm not sure why

@devongovett
Copy link
Member

@jamiebuilds when HMR is not enabled, we dedupe by file contents in the packager:

if (this.dedupe.has(asset.generated.js)) {
return;
}
// Don't dedupe when HMR is turned on since it messes with the asset ids
if (!this.options.hmr) {
this.dedupe.set(asset.generated.js, asset.id);
}

@padmaia
Copy link
Contributor

padmaia commented Jun 29, 2018

This is because Parcel does not fs.realpath file paths as they are resolved, and for good reason: If you have a npm link-d package in your node_modules and you fs.realpath it before resolving its dependencies, then you can get multiple copies of the same dependencies. Basically, Parcel is following Node's --preserve-symlinks option.

This is not quite right. Node’s --preserve-symlinks does not prevent getting multiple copies of the same dependencies.

/repos/parent-package
    ./node_modules
        ./symlinked-package --> /repos/symlinked-package
            ./node_modules
                ./shared-dependency
        ./shared-dependency

Resolving require(‘shared-dependency’) from /repos/parent-package/node_modules/symlinked-package instead of /repos/symlinked-package will give you /repos/parent-package/node_modules/symlinked-package/node_modules/shared-dependency instead of /repos/symlinked-package/node_modules/shared-dependency, but neither of those are equal to /repos/parent-package/node_modules/shared-dependency, so in either case you will wind up with a duplicated shared-dependency . What it does prevent is not being able to resolve a peer dependency of a symlinked package

/repos/parent-package
    ./node_modules
        ./symlinked-package --> /repos/symlinked-package
        ./peer-dep

Interestingly, since modules are cached based on the resolved file name, and --preserve-symlinks changes the resolved filename from the realpath to the requested path, Node would actually consider require('symlink') and require(‘target’) in @jamiebuilds example to be different modules.

That means that considering a file and a symlink of that file to be the same module while also resolving dependencies from symlinked paths would be a break away from Node, which is probably something we want to avoid, at least by default (tests behaving differently from bundles is not the best). If we do want to accommodate those unique scenarios I think it would make more sense as a non default Resolver that you can opt to plugin, which is something I’m hearing would be possible in Parcel@2.

I do think it would good to enable people to use Node’s normal module resolution method though. @devongovett, maybe we should add an option for that? Also, just to put my personal opinion out there, I think it makes more sense to default to using Node’s normal resolution method. I think people might be surprised that Parcel is using --preserve-symlinks by default when normally they would have to opt in to it. Most people probably don’t even know it’s an option and could easily encounter the tests behaving differently than bundles situation. That would have to be something to consider for the next major version though.

@jamiebuilds
Copy link
Member Author

I think people might be surprised that Parcel is using --preserve-symlinks by default when normally they would have to opt in to it.

Yeah, as much as resolution in every tool sucks right now. I think it's best to follow what others are doing and treat every minor deviation as a bug.

With custom resolvers in Parcel 2 we can experiment with what a better model would be. If we figure out something significantly better. We can consider making it the default in the future.

@nolandg
Copy link

nolandg commented Jan 31, 2019

Is there any workaround for this? The only way to work with a local package that has peer deps is to sym link them in. But any package that has say React as a peer dep now gets many copies of React when using Parcel.

Eg: my-app depends on react as well as my-lib local package. my-lib has react as a peer dep, sym linked to my-app/node_modules. If you build my-app with Parcel then the bundle has 2 copies of react and you have now opened a rift in space-time.

In Webpack, I make an alias for all peer deps to force them to use my-app's version. Parcel seems to be ignoring my attempts to alias modules in package.json.

Is there a workaround?

@nolandg
Copy link

nolandg commented Feb 1, 2019

Ahh. I see how the alias syntax works now. You need to be more explicit with the path. Module name alone does not resolve to root project's node_modules. You need to actually specify the absolute or relative-to-project path in the alias and then Parcel uses that for peer deps. So that's the work around for me.

This was referenced Mar 9, 2019
@torbensky
Copy link

torbensky commented Apr 22, 2019

Parcel's deviation from the standard default node module resolution strategy (using real path) is a problem for me. I can't use Parcel with PNPM and originally raised a question about it in the community forum here:

https://spectrum.chat/parcel/general/parcel-pnpm-workspaces-doesnt-work~fe7aa206-92d4-4d08-8c26-4196815f7fad

I've since been debugging Parcel's custom module resolution strategy and can see the problem. It's not using the real path so it cannot find the transitive dependencies which are located relative to the direct dependency (following the link).

@codefrau
Copy link

This might be the root cause for #1838 ?

@cie
Copy link

cie commented May 22, 2019

Until #2946 is merged, pnpm users can use the --shamefully-flatten option of pnpm.

@sadsa
Copy link

sadsa commented Sep 17, 2019

@devongovett - Can we get this PR merged please? Using parcel-bundler with PNPM is currently not possible unless we use the --shamefully-flatten option param. Which completely destroys any performance benefit we get from using it over NPM

@jamiebuilds
Copy link
Member Author

@sadsa This is not a pull request, and it's not a simple problem.

@antl3x
Copy link

antl3x commented Dec 3, 2019

This should be closed since #3471 or only after official v2 release?

@github-actions
Copy link

github-actions bot commented Jun 1, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs.

@github-actions github-actions bot added the Stale Inactive issues label Jun 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 Bug Stale Inactive issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants