Upgrading nextjs to 12.3 - #1103
Closed
wonjun-opensource wants to merge 22 commits into
Closed
Conversation
…perties are valid:"
…n use jsconfig.json
…ed to disable SWC as replacement for Babel because of custom Babel configuration by using .babelrc
…abled css loading from a url by default. url import is an experimental feature we can enable, if needed, https://nextjs.org/docs/api-reference/next.config.js/url-imports
…ess.env.__NEXT_REACT_ROOT' NEXT_RUNTIME
wonjun-opensource
marked this pull request as ready for review
September 18, 2022 23:21
wonjun-opensource
requested review from
shahthepro and
sparrowDom
as code owners
September 18, 2022 23:21
Member
|
@wonjun-opensource thank you for this awesome work and in depth research in to things. Wanted to deploy it to staging to test out sentry but it has failed: I need to look into that. And like you point out we still need to solve the FBT issue probably by migrating to something better supported. |
…his module. Expected version ">=12.22.0".
Contributor
Author
|
@sparrowDom Thank you for the review, It says NextJs minimum is 12.22.0, so I updated README. [Edit] by adding in package.json Could you try staging build again when you get a chance? |
# Conflicts: # dapp/package.json # dapp/yarn.lock
Member
|
Hey sorry for super late reply. Failed when tried to deploy with: Then I've changed it to: and deploy also failed with: |
# Conflicts: # dapp/next.config.js
Contributor
Author
|
@sparrowDom Thanks for trying that. Sorry about the late reply, too. I see in both error messages you shared, it indicates that node 14.x was not installed successfully. I pushed a change setting "node": ">= 14.9" based on GoogleCloudPlatform/nodejs-docker#214 (comment) I hope that fixes the error you saw. Contracts Fork Tests fail as it is also failing in the master. Changes here are limited to /dapp folder only. |
|
@sparrowDom What are your thoughts? Shall we move forward with this upgrade? Looks like the Contracts Fork Tests failed - so we should check on that before merging. |
Member
|
yeah the contract fork test failing isn't relevant and/or a problem. I have added a build github workflow that tests if the production build succeeds. Testing the development speed (which was one of the main highlights of next12+) the results are pretty disappointing on my M1 Mac. Nextjs v10 from initial startup in console to first page render takes ~32 seconds. NextJs v12 takes 45 seconds and NextJs v13 takes 42. It could be the issue is because of Mac silicone. @shahthepro you have an intel mac right? When convenient could you maybe benchmark the speed of first page render comparing NextJs on master ( v10 ) and the one in this branch ( v13 ). If there are no speed gains we will just close the PR. |
Member
|
Also NextJs 13 requires node v14+ and Google GAE flexible environment has the node fixed to v12. There is a custom environment with a Docker workaround, but I haven't managed to get that working yet. And will stop with efforts until we get a clear metric of this NextJs being faster on at least some machines. |
Member
|
I think we can table this one for now |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #894
Upgrades nextjs to 12.3, and React is now at 18.2.
OUSD site is fully rendering, and Swap & Wrapping work as expected on the local dev environment.
Module alias now moves to jsconfig.json out of package.json. Nextjs handles it without the need for
"babel-plugin-module-resolver"in package.json.Nextjs 12 disabled css loading from a url by default. url import is an experimental feature we can enable, if needed, https://nextjs.org/docs/api-reference/next.config.js/url-imports
TODO: Sentry does not run on the local dev environment, so this needs to be tested on staging.
New code changes trigger the following warnings in Terminal,
=> SWC that comes with nextjs 12.3 does not handle "babel" ordering we have in package.json, specifically "babel-plugin-fbt", "babel-plugin-fbt-runtime", which needs to do the transforms before other transforms beat to the Babel AST, so I moved "presets" and "plugins" to .babelrc, and that disables SWC for now.
If we want to re-enable SWC, we may need to wait for SWC to add better support for it, or handle it with something like this they suggested, https://facebook.github.io/fbt/docs/getting_started_on_web/#babel-plugin-ordering-caveat.
fbt-runtimeis no longer supported, so we should consider changing it to usingfbtornext-i18next, which seems more compatible with nextjs, possibly without the same problemfbt-runtimegives.=> @myetherwallet/mewconnect-web-client is no longer supported. We may be able to suppress this type of warning. I left it as it is now.
=> We either need to suppress this type of warning or wait for the package to be updated. The latest version still triggers this warning, so I kept the existing version.