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

bundledDependencies not working #844

Closed
vjpr opened this issue Jul 14, 2017 · 11 comments
Closed

bundledDependencies not working #844

vjpr opened this issue Jul 14, 2017 · 11 comments

Comments

@vjpr
Copy link
Contributor

vjpr commented Jul 14, 2017

Try install pnpm i -g dimsim-docker@0.1.2, then run dimsim-docker.

Error: Cannot find module 'core-js/library/fn/symbol/iterator'
    at Function.Module._resolveFilename (module.js:485:15)
    at Function.Module._load (module.js:437:25)
    at Module.require (module.js:513:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (C:\Users\Vaughan\AppData\Roaming\nvm\v8.1.2\pnpm-global\1\node_modules\.registry.npmjs.org\dimsim-docker\0.1.2\nod
e_modules\dimsim-docker\node_modules\babel-runtime\core-js\symbol\iterator.js:1:93)
    at Module._compile (module.js:569:30)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:503:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)

babel-runtime is a bundled dep but does not get its dependencies (core-js) installed correctly.

I am seeing the same issue with https://github.com/mapnik/node-mapnik` which uses node-pre-gyp as a bundled dep.

The issue doesn't show up when you run a pnpm i (in dimsim-docker for example) - as they are installed as normal dependencies. It only shows up when it is being globally installed, or when the package with bundledDeps is a transitive dep.

@vjpr
Copy link
Contributor Author

vjpr commented Jul 14, 2017

npm/npm#9131 (comment):

if a package is marked as a bundledDependency, that tells npm to copy everything logically under that package in the dependency tree into the tarball at npm pack time.

This is also why npm@3 figures out what to bundle from the logical (not filesystem) tree – to keep this behavior consistent and usable across all versions of npm that support bundling.

Seems like it was simply not implemented in pnpm?

@zkochan
Copy link
Member

zkochan commented Jul 14, 2017

pnpm just skips the bundled dependencies, so it does not install packages that are listed in bundledDependencies. I am not entirely sure how npm@3 handles this. It skips the top dependencies but installs the dependencies of bundled dependencies?

@vjpr
Copy link
Contributor Author

vjpr commented Jul 14, 2017

From this comment...

This is also why npm@3 figures out what to bundle from the logical (not filesystem) tree – to keep this behavior consistent and usable across all versions of npm that support bundling.

...it looks like they will create a traditional nested node_modules structure for that dep when packing.

We should do similar.

@vjpr
Copy link
Contributor Author

vjpr commented Jul 14, 2017

But this would mean that pnpm would need to be used for pnpm pack and pnpm publish would need to be used.

Or we could include a function that should be added as a pre-publish step, to ensure bundled deps are bundled properly before publishing a package with bundledDeps.

@zkochan
Copy link
Member

zkochan commented Jul 14, 2017

oh, so the problem is with a package published by pnpm.

Currently pnpm does not support publishing of packages that have bundled dependencies at all (see "Limitations" section in README). Even though we don't print any warnings about it. We should.

It should work though with packages that have bundled deps but are published via npm. A weird situation I guess...

I think if we want bundled deps in pnpm, we should do it the pnpm way, which is, we need them in a symlinked node_modules. Unfortunately, that means the pnpm bundledDependencies would be incompatible with npm and Yarn. We'd have to use a new property name and npm/Yarn would then just fallback to installing the "pnpm bundeledDependencies" as regular dependencies

@zkochan
Copy link
Member

zkochan commented Jul 14, 2017

In any case, as a first step, pnpm should fail if someone wants to publish a package with bundedDependencies

@vjpr
Copy link
Contributor Author

vjpr commented Jul 14, 2017

oh, so the problem is with a package published by pnpm.

There are 2 cases:

  1. Where it is published by pnpm - I am fine to avoid bundledDeps so I don't really care about this one much - and I think its fine to leave it as a limitation.

  2. Where it is published by npm - This needs to be supported because having just 1 module with bundled deps in a tree would break the whole app mysteriously.

@vjpr
Copy link
Contributor Author

vjpr commented Jul 14, 2017

In any case, as a first step, pnpm should fail if someone wants to publish a package with bundedDependencies

Agree. But often I will use npm publish to publish anyway and a lot of publish-helper tools would do the same. So it should be a recommendation to use a pre-publish script which shows the error.

@lukescott
Copy link

lukescott commented Feb 1, 2019

With normal dependencies being stored at node_modules/.registry.npmjs.org/ could bundled dependencies be stored at node_modules/.bundled/ or something similar? The idea is to treat the bundled dependencies like they come from a different registry.

With pack the nested node_modules might be somewhat of an issue, and may not be ideal. Perhaps you could add a bundle command. It would behave similarly to pack, but would store bundledDependencies inside node_modules/.bundled/. pnpm would then create the symlinks on install.

Either way it would also be nice to have it work with workspaces. For example, I have a package within the workspace that represents an app. It uses several utility packages. I would like to bundle that app with the workspace dependencies for a nightly build.

@zkochan
Copy link
Member

zkochan commented May 7, 2022

This is resolved. Packages with bundled dependencies can be installed by pnpm.

If you want to publish a package with bundled deps, you need to use node-linker=hoisted

@zkochan zkochan closed this as completed May 7, 2022
@VasilyGerrans
Copy link

Sorry to reawaken this issue. I have tested bundling dependencies with node-linker=hoisted in the .npmrc file. While a pnpm project bundles as you would expect when installing it as a dependency in a npm project, it does not bundle dependencies into its node_modules folder the way you would expect when installing it into a pnpm project.

Is this expected behaviour?

If not, here is a minimal repo that reproduces this case, consisting of a package.json with a single pnpm dependency which has .npmrc with node-linker=hoisted in it: https://github.com/VasilyGerrans/erc7579-implementation

After cloning the repo , run pnpm i and inspect node_modules to reproduce the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants