plouc / nivo

nivo provides a rich set of dataviz components, built on top of the awesome d3 and React libraries
https://nivo.rocks
MIT License
13.23k stars 1.03k forks source link

0.83.0 Doesn't work with pnpm + webpack #2401

Closed ollwenjones closed 8 months ago

ollwenjones commented 1 year ago

Describe/explain the bug When using 0.83.0 with wepback + pnpm, something goes wrong in the dep tree, and the result is that all the Container.js contexts hooks produce undefined values wherever they are called, resulting in lots of runtime errors and blank charts.

Runtime Errors are like

Cannot read properties of undefined (reading 'animate')
    TypeError: Cannot read properties of undefined (reading 'animate')

To Reproduce Does not reproduce using npm, but switching this repo from npm to pnpm reproduced it immediately.

https://github.com/ollwenjones/nivo-0830-test

Expected behavior Since nivo uses pnpm internally (per contributing page) one would expect it to work with pnpm

Screenshots

Screenshot 2023-07-24 at 4 06 06 PM

Desktop (please complete the following information):

Additional context Really tried to solve this, and will contribute a fix if I can figure it out, but had to timebox it. My guess is some dependency problem caused by pnpm isolating modules is being masked by these react-contexts-are-blank runtime errors. Tried to find a duplicate version of React, but was unable to in the time that I had.

francoisjacques commented 1 year ago

I've hit that too. Exactly same symptoms, within a nextjs 13 (.4 and .5) application. Stack uses typescript as well (bundler module resolution). Switching to npm fixed it, but led me into a conendrum of trying to adjust our yarn/pnpm monorepo into an hybrid.

Gave up and decided of not using nivo on this project for now as we're still prototyping.

Really interested to get to the bottom of it and subscribed to the issue.

mattoni commented 1 year ago

Same issue here. Getting a lot of undefined errors around the useMotionConfig() hook.

unrealsolver commented 1 year ago

The same issue happens with Yarn 3 (Berry / PnP)

Pra3t0r5 commented 1 year ago

Same issue, and, except for chrome, exact same env

nzben commented 1 year ago

Likewise, yarn + nextjs13. I've tried resolving dependencies in yarnrc but am coming up blank.

OsmelSynData commented 1 year ago

same issue here after upgrade to react 18 and yarn 3. any updates or workarounds?

ollwenjones commented 1 year ago

@OsmelSynData unfortunately the workaround for me was to postpone upgrading Nivo. We are still on 0.69.1.

I wonder if some dependency graph too like https://github.com/pahen/madge could point to where a different version of React is getting pulled in, or something... 🤔

OsmelSynData commented 1 year ago

@OsmelSynData unfortunately the workaround for me was to postpone upgrading Nivo. We are still on 0.69.1.

I wonder if some dependency graph too like https://github.com/pahen/madge could point to where a different version of React is getting pulled in, or something... 🤔

Well actually, I managed to find work around the problem by not upgrading to yarn berry.

"@nivo/core": "0.83.0" "react": "^18.2.0", yarn --version => 1.22.19

Seems to be working really smoothly actually. Hope this works!

marvinruder commented 10 months ago

I have the same issue with Yarn 4 and Plug'n'Play – when using the nodeLinker: node-modules configuration, everything works fine. But this is no viable option in my project, so I would really like to see this issue fixed. Is there anything we can do to help?

mrclrchtr commented 9 months ago

Same here... so we can't use the lib.

marvinruder commented 8 months ago

It would appear that at least for Yarn PnP a workaround is to add all peer dependencies of @nivo/core as its own dependencies in the .yarnrc.yml file:

packageExtensions:
  "@nivo/core@*":
    dependencies:
      prop-types: "*"
      react: "*"

Allow me to explain.

While analyzing this issue, I realized that the Chrome debugger showed two instances of the motionConfigContext in the module scope. One was holding the correct value, the other’s value was undefined. This was apparently caused by two different instances of @nivo/core being present, its paths looked like .yarn/__virtual__/@nivo-core-[hash-1]/[path to the same @nivo/core zip file in global cache] and .yarn/__virtual__/@nivo-core-[hash-2]/[path to the same @nivo/core zip file in global cache]. Those virtual packages are described by Yarn here:

Because peer-dependent packages effectively define an horizon of possible dependency sets rather than a single static set of dependencies, a peer-dependent package may have multiple dependency sets. When this happens, the package will need to be instantiated at least once for each such set.

Since in Node-land the JS modules are instantiated based on their path (a file is never instantiated twice for any given path), and since PnP makes it so that packages are installed only once in any given project, the only way to instantiate those packages multiple times is to give them multiple paths while still referencing to the same on-disk location. That's where virtual packages come handy.

Virtual packages are specialized instances of the peer-dependent packages that encode the set of dependencies that this particular instance should use. Each virtual package is given a unique filesystem path that ensures that the scripts it references will be instantiated with their proper dependency set.

(One can observe the different virtual packages by looking into the .pnp.cjs file Yarn generates. The packagePeers array lists the peer dependencies for each instance.)

So although those different paths are pointing to the exact same source code, they are not treated as the same thing in the browser, which led to undefined context values. A simple workaround for this is to change the dependencies of @nivo/core as described abobe so that only one possible dependency set remains and the package is only instantiated once.

Depending on the @nivo/* packages in use, smaller workarounds are possible. In my project, I only use @nivo/sunburst which makes use of @nivo/tooltip. @nivo/sunburst has react as peer dependency, @nivo/tooltip has no peer dependencies at all. Apparently, @nivo/sunburst then used the context provider from the @nivo/core instance with peer dependency react and @nivo/tooltip used the context consumer from the @nivo/core instance without peer dependency react. The instances were different, the contexts were different and the consumer could not consume what the provider provided. To fix this, I only had to declare the additional peer dependency react for @nivo/tooltip and both packages used the same instance of @nivo/core:

packageExtensions:
  "@nivo/tooltip@*":
    peerDependencies:
      react: "*"

I am not sure whether this is a bug in the (peer) dependency configuration of @nivo/* packages or a bug in Yarn PnP, and there might be other workarounds that lead to the same package instance being used in all possible places. I am however fairly certain that declaring the same peer dependencies for all packages in this repository that depend on @nivo/core should fix the issue for everyone (at least with Yarn PnP, I have no idea how pnpm works).

marvinruder commented 8 months ago

And since we are already talking about (peer) dependencies: Working with @nivo/core in Yarn PnP strict mode without declaring prop-types as a dependency of @nivo/core throws an error:

The Yarn Plug'n'Play manifest says this package has a peer dependency on "prop-types", but the package "prop-types" has not been installed

But I added prop-types to my package’s dependencies. To me, it seems that peer dependencies do not work when nested. If A depends on B and B depends on C and C has a peer dependency on D, B must either declare D as its dependency or peer dependency, forcing A to add D as a dependency in the latter case. It is not sufficient if only A declares D as its dependency on its own without D appearing in B at all.

In the case of this repository, every package depending on @nivo/core must declare all peer dependencies of @nivo/core, namely prop-types and react, either as their dependencies or peer dependencies.

(This requirement would then also extend to all transitive dependencies: @nivo/express depends on @nivo/static depends on @nivo/core, so it would need to declare all peer dependencies of @nivo/core as well without directly depending on it, simply because @nivo/static would get new peer dependencies in the first place.)

ollwenjones commented 8 months ago

@marvinruder this is a great insight into the problem. Thank you for the time spent digging into this!

SteveAtKlover commented 8 months ago

i am having this issue too. any time i hover over a chart now it breaks the page and throws this console error. i just upgraded to react 18. will try downgrading to an earlier version for now, but please fix.

marvinruder commented 8 months ago

i am having this issue too. any time i hover over a chart now it breaks the page and throws this console error. i just upgraded to react 18. will try downgrading to an earlier version for now, but please fix.

@SteveAtKlover Which version of nivo are you experiencing this issue with? It should be fixed in 0.85.0. Perhaps you might want to share your project's package.json and lockfile or an MWE repo?

SteveAtKlover commented 8 months ago

@marvinruder using 0.85.1

files attached package-lock.json package.json

marvinruder commented 8 months ago

@marvinruder using 0.85.1

files attached package-lock.json package.json

Thanks! I see nothing wrong with the dependencies themselves, only one version of react (18.2.0) is pulled in. I quickly combined your dependencies with Nivo's example setup and experienced no issues at all.

Which package manager are you using (I tested with pnpm@8)? Maybe you can create and share an MWE setup reproducing the issue?

SteveAtKlover commented 8 months ago

I'm using npm. I'll see if I can spin something up

On Thu, Mar 28, 2024, 1:26 PM Marvin A. Ruder @.***> wrote:

@marvinruder https://github.com/marvinruder using 0.85.1

files attached package-lock.json https://github.com/plouc/nivo/files/14794656/package-lock.json package.json https://github.com/plouc/nivo/files/14794657/package.json

Thanks! I see nothing wrong with the dependencies themselves, only one version of react (18.2.0) is pulled in. I quickly combined your dependencies with Nivo's example setup https://github.com/plouc/nivo/tree/master/examples/codesandbox and experienced no issues at all.

Which package manager are you using (I tested with @.***)? Maybe you can create and share an MWE setup reproducing the issue?

— Reply to this email directly, view it on GitHub https://github.com/plouc/nivo/issues/2401#issuecomment-2026054568, or unsubscribe https://github.com/notifications/unsubscribe-auth/AYDS5DEBVU2MAHKSRMGSW53Y2RVFVAVCNFSM6AAAAAA22EZMC6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRWGA2TINJWHA . You are receiving this because you were mentioned.Message ID: @.***>

msdrigg commented 6 months ago

For those coming through now, I saw this error when I accidentally installed @nivo/core version 0.85.1 and @nivo/pie version 0.86.0.

Just give a quick check for dependencies between various @nivo/* dependencies to make sure all match.