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

0.83.0 Doesn't work with pnpm + webpack #2401

Closed
ollwenjones opened this issue Jul 27, 2023 · 19 comments · Fixed by #2528
Closed

0.83.0 Doesn't work with pnpm + webpack #2401

ollwenjones opened this issue Jul 27, 2023 · 19 comments · Fixed by #2528

Comments

@ollwenjones
Copy link
Contributor

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):

  • OS: Building on Mac OS or Windows, doesn't matter here.
    • Node 16, pnpm 17, webpack 5 (setup per Create React App)
  • Browser: Chrome
  • Version 114.0.5735.198

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
Copy link

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
Copy link

mattoni commented Sep 24, 2023

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

@unrealsolver
Copy link

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

@Pra3t0r5
Copy link

Pra3t0r5 commented Oct 11, 2023

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

@nzben
Copy link

nzben commented Oct 16, 2023

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

@OsmelSynData
Copy link

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

@ollwenjones
Copy link
Contributor Author

@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
Copy link

OsmelSynData commented Oct 25, 2023

@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
Copy link
Contributor

marvinruder commented Jan 10, 2024

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
Copy link

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

marvinruder added a commit to marvinruder/rating-tracker that referenced this issue Mar 5, 2024
….yml`

* Fixes plouc/nivo#2401 (for whatever reason)

Signed-off-by: Marvin A. Ruder <signed@mruder.dev>
marvinruder pushed a commit to marvinruder/rating-tracker that referenced this issue Mar 5, 2024
* Add peer dependencies of `@nivo/core` as its dependencies in `.yarnrc.yml` as a workaround for plouc/nivo#2401 (no idea why this works)

Signed-off-by: Marvin A. Ruder <signed@mruder.dev>
@marvinruder
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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

plouc pushed a commit that referenced this issue Mar 6, 2024
* Fix (peer) dependencies

* Declare `prop-types` as a dependency of `@nivo/core` (as recommended in their README)
* Declare `react` as a peer dependency of `@nivo/tooltip`
* Update lockfile

Fixes #2401

Signed-off-by: Marvin A. Ruder <signed@mruder.dev>

* Use and declare the same `pnpm` version as `ci/codesandbox` uses

Signed-off-by: Marvin A. Ruder <signed@mruder.dev>

---------

Signed-off-by: Marvin A. Ruder <signed@mruder.dev>
@SteveAtKlover
Copy link

SteveAtKlover commented Mar 28, 2024

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
Copy link
Contributor

marvinruder commented Mar 28, 2024

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
Copy link

@marvinruder using 0.85.1

files attached
package-lock.json
package.json

@marvinruder
Copy link
Contributor

@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
Copy link

SteveAtKlover commented Mar 28, 2024 via email

@msdrigg
Copy link

msdrigg commented May 9, 2024

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.

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

Successfully merging a pull request may close this issue.