-
-
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
fix: reduce deprecated api and node warnings #183
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 955202f:
|
I understand the Node 18 warning, but how the Node 20 warning happens? Are we not use ESM imports? |
This is complex. i read the nodejs source code. Reason is conditions will not pass to the esm module in worker thread. BUT, cjs will. So in the first time, require(react) is in correct condition(used in webpack source code). Second time when you import react-server-webpack, it’s not react-server condition actually |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks good to me. My hesitation is that I will not be able to remember that this change is just about reducing warnings.
Can you add source code comments which include the link to this PR?
9b5a35c
to
cdf0fc8
Compare
fb4779d
to
104b671
Compare
https://github.com/dai-shi/waku/actions/runs/7012521326/job/19077187950?pr=183 |
There might be some flaky. or in some cases, CI is slower than usual cause the build is overdue. Let me debug it |
could you run that again with debug log? I will go to dinner now. do this later |
Will rebase this later |
b3fce66
to
955202f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fix the warning on node.js 20
Upstream: nodejs/node#50885
And deprecated API on node.js 18