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

dedupe leads to errors in a monorepo with hoisted dependencies #86

Closed
jpaquim opened this issue Dec 9, 2019 · 7 comments · Fixed by #98
Closed

dedupe leads to errors in a monorepo with hoisted dependencies #86

jpaquim opened this issue Dec 9, 2019 · 7 comments · Fixed by #98

Comments

@jpaquim
Copy link

jpaquim commented Dec 9, 2019

  • Rollup Plugin Name: @rollup/plugin-node-resolve
  • Rollup Plugin Version: 6.0.0
  • Rollup Version: 1.27.9
  • Operating System (or Browser): macOS
  • Node Version: 13.2.0

How Do We Reproduce?

Here is a repo reproducing the issue: https://github.com/jpaquim/rollup-monorepo-issue

Steps:

  • Start with a monorepo configuration using package hoisting (the default for yarn workspaces, or using lerna with hoisting enabled)
  • Setup rollup and @rollup/plugin-node-resolve in one of the monorepo's packages (let us assume it's in a directory under packages/).
  • Configure @rollup/plugin-node-resolve to dedupe a certain package (in my case, I was using sveltejs/template, with dedupe: importee => importee === 'svelte' || importee.startsWith('svelte/') or more simply dedupe: ['svelte', 'svelte/internal'].
  • Import the given package in one of the source files
  • Bundle using rollup, using as working directory the package directory, and not the monorepo root.

Expected Behavior

The imported package should be correctly bundled with the rest of the output.

Actual Behavior

The imported package is not resolved correctly, rollup complains: (!) Unresolved dependencies https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency svelte/internal (imported by src/App.svelte)

Consequently, the import statement remains in the output. In my case: import { SvelteComponent, init, safe_not_equal, element, text, space, attr, insert, append, set_data, noop, detach } from 'svelte/internal';

Comments

The issue seems to be related to this line, where the importee is overwritten with join(process.cwd(), 'node_modules', importee).

In a monorepo context, process.cwd() will typically refer to the package where the rollup command is started from, but with hoisting, the relevant node_modules are actually at the root of the monorepo, thus leading to the conflict.

@shellscape
Copy link
Collaborator

Thanks for the issue 🍺

This is going to be a difficult one to remedy in a monorepo. I'm afraid the setup is going to be a bit complex in order to get multiple folks working on triage. Can you provide a repo that has a reproducible setup? One with the setup you're using would be most helpful.

@jpaquim
Copy link
Author

jpaquim commented Dec 9, 2019

@shellscape sure thing, here is a repo reproducing the issue: https://github.com/jpaquim/rollup-monorepo-issue

Run yarn at the root, then navigate to packages/template and run yarn build, and you should see the resulting (!) Unresolved dependencies https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency svelte/internal (imported by src/App.svelte) warning.

@shellscape shellscape changed the title @rollup/plugin-node-resolve - dedupe leads to errors in a monorepo with hoisted dependencies dedupe leads to errors in a monorepo with hoisted dependencies Dec 9, 2019
@jpaquim
Copy link
Author

jpaquim commented Dec 9, 2019

I don't have enough context on the internals of rollup or this plugin, so this will probably be naïve, but it seems that just replacing join(process.cwd(), 'node_modules', importee) with require.resolve(importee) seems to work for this specific case, basically letting node itself resolve the importee from the parent folders recursively.

However, I'm not sure if this would actually work inside a nested package, what the behaviour of require.resolve would be in that situation...

@shellscape
Copy link
Collaborator

#34 is likely to affect this as well.

@LarsDenBakker
Copy link
Contributor

I've created a PR for this #98

@jpaquim
Copy link
Author

jpaquim commented Dec 25, 2019

Great, thank you @LarsDenBakker and @shellscape for the great work!

@shellscape
Copy link
Collaborator

👍 we're still working on getting publishing smoothed out, so please have patience on that getting to NPM

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

Successfully merging a pull request may close this issue.

3 participants