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

Default "include" option fails for monorepos #17

Open
fionawhim opened this issue Nov 5, 2020 · 2 comments
Open

Default "include" option fails for monorepos #17

fionawhim opened this issue Nov 5, 2020 · 2 comments

Comments

@fionawhim
Copy link

fionawhim commented Nov 5, 2020

[heavily edited to match what the source of the bug is]

The default include option that’s passed to rollup-plugin-inject, 'node_modules/**/*.js', causes problems when working in a monorepo where node_modules dependencies are hoisted above the current working directory where rollup is run.

The include ends up getting resolved relative to the current directory, even though the Node resolution might find modules in a node_modules directory in a parent.

For me, this manifested in the buffer-es6.js’s global not getting converted into its own polyfill, which caused an Uncaught ReferenceError: global is not defined error.

A workaround is to manually specify an include option to the rollup-plugin-node-polyfills.

@fionawhim
Copy link
Author

Hrm. This issue might actually be “why doesn’t @rollup/plugin-inject correctly replace global when Rollup is run by Snowpack / esinstall.

Sorry I might have jumped the gun filing thing this. Digging more.

@fionawhim fionawhim changed the title buffer-es6 polyfill references global Default "include" option fails for monorepos Nov 5, 2020
@fhd
Copy link

fhd commented Feb 2, 2021

@fionawhim You just saved me a lot of time trying to get to the bottom of this, thank you!

It seems rollup-plugin-inject is not following symlinks either, but the following workaround did it for my case (with all linked packages being directly in the repository root):

import nodePolyfills from "rollup-plugin-node-polyfills";
...
    plugins: [
            ...
            nodePolyfills({
                include: '../**/node_modules/**/*.js'
            }),
            ...
    ]

It's a bit nasty, but a lot better than my previous workaround :D

fhd added a commit to polypoly-eu/polyPod that referenced this issue Feb 2, 2021
This was a real nasty one! With no error, for no apparent reason, `container.js`
would contain references to `process` (the Node.js built-in), called by a
transitive dependency of podigree.

The relevant change is in rollup.config.js, the rest is babbling. Turns out
rollup-plugin-node-polyfills (which is unmaintained...) hard codes the path to
node_modules, and doesn't follow symlinks within node_modules. Charming... The
current workaround fixes it for now, but for Yarn 2's Plug'n'Play linking, we'd
need another workaround yet.

There's an issue in the rollup-plugin-node-polyfills project for this, but since
it's dead, it probably won't be fixed. Still, the author of that issue probably
saved us hours:

ionic-team/rollup-plugin-node-polyfills#17

Important: There is currently no regression test for this. Presumably the
orodruin/example tests will cover this (once fixed), but for now it can only be
tested manually. Once those tests are fixed, we need to see if reverthing this
fix breaks them, as expected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants