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

dev: error when using locally linked version of waku #676

Closed
pmelab opened this issue Apr 27, 2024 · 19 comments · Fixed by #1084
Closed

dev: error when using locally linked version of waku #676

pmelab opened this issue Apr 27, 2024 · 19 comments · Fixed by #1084

Comments

@pmelab
Copy link
Contributor

pmelab commented Apr 27, 2024

I'm trying to link a local version of Waku into our own project code to quickly iterate on PR's and test them in a real use case, but I keep getting the follow error:

TypeError: Cannot read properties of null (reading 'use')
    at react_production_min.use ([...]/waku-test/dist/ssr/assets/rsc0-b97afaf68.js:324:22)
    at Slot ([...]/waku-test/dist/ssr/assets/rsc0-b97afaf68.js:10168:40)

Can be reproduced by:

git clone [email protected]:dai-shi/waku.git
cd waku
pnpm install
pnpm compile
cd ..
pnpm create waku # choose name 'waku-test'
cd waku-test
pnpm remove waku
pnpm add ../waku/packages/waku
pnpm build

Did anybody encounter this or have an idea what could cause it?

@dai-shi
Copy link
Owner

dai-shi commented Apr 27, 2024

How about pnpm link?

@pmelab
Copy link
Contributor Author

pmelab commented Apr 27, 2024

🙈 I completely forgot that pnpm link exists. Thank you for jogging my mind!

But it yields the same result.

@dai-shi
Copy link
Owner

dai-shi commented Apr 27, 2024

Yeah, now I remember that I probably tried it before. (I wonder if @himself65 has any idea.)

It probably fails with similar issue like #429.
We want to fix it and your investigation is welcome.

For the moment, the way it works is to use npm (or maybe yarn) and replace node_modules/waku/dist. You need to copy the files because symbolic link doesn't work.

@pmelab
Copy link
Contributor Author

pmelab commented Apr 27, 2024

I have to integrate it into a monorepo setup as well. I will keep an eye open and report back.

@JesseKoldewijn
Copy link
Contributor

Did ya try to use workspaces instead of links? (ie. pnpm workspaces)

@himself65
Copy link
Contributor

I think this is because there are two different versions of React; could you please confirm that?

@himself65
Copy link
Contributor

Just double checked and yes it is

@tylersayshi
Copy link
Contributor

tylersayshi commented Dec 5, 2024

@pmelab is this still an issue for you? from what @himself65 mentioned it should be resolved from using a single version of react.

in case it's helpful... pnpm catalogs are a handy newer pnpm tool for manager versions of react at the root of your monorepo: https://pnpm.io/catalogs

@tylersayshi
Copy link
Contributor

oh sorry I was able to reproduce this easily with the description instructions. I should have tried that first. Please disregard the above ^

@tylersayshi
Copy link
Contributor

this can be resolved with adding the following vite.config.ts to waku-test

import { defineConfig } from "vite";

export default defineConfig({
  resolve: {
    dedupe: ["react", "react-dom"],
  },
});

not sure of the best way to handle this from the waku project itself. @dai-shi do you have any opinion for how we could solve this for everyone or instruct users for how to solve it?

I ask mainly because we have this roadmap item:

single waku.config.ts config file (no vite.config.ts)

would waku.config.ts theoretically extend vite.config.ts so users could configure things like this there?

A solution idea

we could potentially create presets for waku.config.ts at some point and consider this part of a "monorepo preset" if we had enough need for an abstraction. Maybe just docs for this as a common error and how to solve it would be good for now?

@dai-shi
Copy link
Owner

dai-shi commented Dec 5, 2024

Hm, it looks like my bad. I was confused with something else. (#1032 might be closer to something in mind.)

If "two different versions of React" is the only problem and dedupe: ["react", "react-dom"], solves it, just making sure to user the same React version that Waku depends on solves it, doesn't it?

@tylersayshi
Copy link
Contributor

Well the problem here is that waku is added as a dependency from the local filesystem. This would happen whenever running waku linked with a symlink or by installing from the file system protocol. (e.g. `"waku": "../../waku/packages/waku" like from the issue description here).

So now when you are running waku build it uses the node_modules from test-waku (which has react), but ../../waku/packages/waku has it's own node_modules that it gets react from. From running the reproduction above I had matching react versions, but the error still happens, so dedupe is still needed.

When waku is published and used through normal installation from the npm registry, react is marked as a peerDep, so waku does not actually have it's own node_modules/react like it does in the local filesystem setup.


So the summary here is:

When linking or adding a package from local filesystem that depends on react, we need to configure dedupe to react and react-dom

There may be another way to resolve this problem, but this is the method I've relied on for years with running other projects with linked local installed packages that use react.

@dai-shi
Copy link
Owner

dai-shi commented Dec 6, 2024

Thanks for the clarification. I misunderstood it completely. So, it's a symlink issue.
Can we always specify dedupe? Would there be a side-effect with normal installation?

@tylersayshi
Copy link
Contributor

I don't personally know any side effects of always de-duping. The only thing I think it could potentially impact is maybe dev build speeds? Although the difference would not be noticeable I think.

We should ask someone with deeper knowledge of vite before deciding on that I'd think though. Just to be safe.

@dai-shi
Copy link
Owner

dai-shi commented Dec 6, 2024

Let's see if @pmelab can solve his problem with #676 (comment) solution first.

@pmelab
Copy link
Contributor Author

pmelab commented Dec 6, 2024

Yes, I can confirm it works!

And I learned about the dedupe option, which solved two other duplicated-package issues we had as well 😱
Thank you very much!

@rmarscher
Copy link
Contributor

rmarscher commented Dec 6, 2024

I have seen similar errors in production. "Cannot read properties of null" or "null is not an object" and it's always with trying to invoke one of the react hooks (use, useRef, useState, etc).

I agree this is probably caused by multiple versions (or maybe just instances) of react being loaded into memory - so after a deploy to production, if the client doesn't cleanly load all of the updated modules. I opened a discussion about a "version skew" feature for Waku that could help avoid issues when deploying new client code.

I did not know about dedupe either. That is great.

Using package.json overrides seems to be another technique to help make sure that a react 19 compatible version of a dependency is installed (for example, see the note about Recharts at the bottom of this guide https://ui.shadcn.com/docs/react-19).

edit: attaching a screenshot with the start of a stack trace in production for one of these types of exceptions:
Screenshot 2024-12-06 at 12 27 58 PM

I'll add dedupe to my vite config and see if that helps reduce the frequency of these errors at all.

@tylersayshi
Copy link
Contributor

@rmarscher sounds good, thanks for trying that out!

Using package.json overrides seems to be another technique to help make sure that a react 19 compatible version of a dependency is installed

This is true, but if I'm not mistaken, overrides are done differently depending on repo setup (monorepo or not) and also based off which package manager is used. IMO we should avoid opinions on package managers and attempt to fix this with vite if we can (which seems to work well so far)

(for example, see the note about Recharts at the bottom of this guide)

I've been following this recharts issue about react-is... it's useful info here, but worth nothing that recharts as a library cannot have opinions on how to use the bundler. We can have opinions on this and do that by managing the vite config, so dedupe feels like the right mechanism for us here.

@dai-shi
Copy link
Owner

dai-shi commented Dec 8, 2024

so after a deploy to production, if the client doesn't cleanly load all of the updated modules.

It's a different issue from this one. Let's continue discussions in #1037.

Using package.json overrides seems to be another technique

That's what I thought too but it's different from what dedupe solves.


I think this issue can be closed. Only remaining thing is either add the technique in ./docs or always use dedupe in the waku core.

@dai-shi dai-shi linked a pull request Dec 22, 2024 that will close this issue
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.

6 participants