-
-
Notifications
You must be signed in to change notification settings - Fork 128
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
Trying to upgrade and getting errors #766
Comments
The build step
Can you make sure to use I'm not sure if it relates to the error though. The monorepo setup seems tough. |
I think the real problem is the errors can be really hard to tie to something that caused the error I fixed up those errors and still get the same runtime error in So it seems like in this case it's having trouble rendering the client component |
In all cases the bug is complaining about a prop. In the case of the table cell it doesn't like |
Okay that prop was getting passed to the dom, and that seems to break something If I omit the prop the app renders fine |
interesting as recent changes are basically for the server. |
from server components? So, is it easy to create a very small reproduction? |
oh, okay.
Yeah, i've also seen it before. Not sure how it causes it yet. |
yeah I can try to pare it down |
Okay got it down to just 1 file. hopefully I didn't miss any setup steps Setup should be
Then navigate to When I remove the context it seems to work (but I want the context 😅 ) |
I was hoping to run it on StackBlitz: But, it can't install deps in the subfolder. 😅 |
Hm, I can reproduce the error locally.
But, it sounds like we could make a smaller repro, even without monorepo. We have an e2e test with |
i've simplified it further too I'll remove all the monorepo stuff if that helps |
monorepo stuff removed |
Okay simplified it as much as I think I can. It seems to have to do with the re-use of |
This is great!!! But, I wish more if it can fit in e2e fixture. |
Hmm, one decorator is a server component and the other is a client component. |
Yeah the decorators are needed and the way I compose them is too |
I think it can be as simple as: <PageContextProvider value={page}>
<ClientHolder page={page}>
<ServerHolder page={page}>
<ClientButton />
</ServerHolder>
</ClientHolder>
</PageContextProvider> Or something like that. (Well "simple" might be subjective) |
i'm finding I can only repro if the loop is there |
So, it looks like when we pass |
Oh, really. That's interesting. |
I was wrong. what's important is the order of the decorators CenteredDecorator > A11yDecorator (doesn't work) |
example is just this now import { StoryContextProvider } from "../../components/Context";
import A11yDecorator from "../../A11yDecorator";
import CenteredDecorator from "../../CenteredDecorator";
export default function Iframe() {
const page = {};
return (
<StoryContextProvider value={{ page }}>
<CenteredDecorator page={page}>
<A11yDecorator page={page}>
foo
</A11yDecorator>
</CenteredDecorator>
</StoryContextProvider>
);
} |
Nice! We could further reduce files and lines, but it's pretty understandable now. Thanks. So, I did this change: diff --git a/package.json b/package.json
index 8ad6a33..763059d 100644
--- a/package.json
+++ b/package.json
@@ -3,9 +3,9 @@
"version": "0.1.0",
"private": true,
"dependencies": {
- "react": "19.0.0-rc.0",
- "react-dom": "19.0.0-rc.0",
- "react-server-dom-webpack": "19.0.0-rc.0",
+ "react": "19.0.0-beta-e7d213dfb0-20240507",
+ "react-dom": "19.0.0-beta-e7d213dfb0-20240507",
+ "react-server-dom-webpack": "19.0.0-beta-e7d213dfb0-20240507",
"waku": "^0.21.0-beta.0"
},
"devDependencies": { And, it works. So, it looks like an upstream issue. You want to find the exact PR in React repo that causes this error and see if it's intentional. |
I was hoping to use `-rc.0` for a while, but it has a bug and `-rc.<num>` has not been maintained. Let's use nightly rc version. close dai-shi#766
I'm trying to upgrade to the latest beta and I get this error when starting my app
Should I report this upstream? This all works on the non-beta version
My repo is here. The upgrade work is on the
upgrade-waku
branch.Set up steps:
Navigate to
localhost:3000/bench
The text was updated successfully, but these errors were encountered: