-
-
Notifications
You must be signed in to change notification settings - Fork 182
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
Upgraded Expo to version 46 and Next.js to version 12.2.x #120
Conversation
Someone is attempting to deploy a commit to a Personal Account owned by @nandorojo on Vercel. @nandorojo first needs to authorize it. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
thank you for this! upgrading a monorepo with expo can be a huge pain so i appreciate it. i’ll review when i have some time. |
Not sure if Next 12.2 is compatible, I was getting this weird error
Which seemed to be affecting the custom |
i believe showtime faced this, could you reference their repo for a solution? |
I believe this is the PR you might be referring to @nandorojo showtime-xyz/showtime-frontend#1261 I can try making it work locally but will need some time. |
thanks guys! i'm super tied up with work but will look when i can. the showtime document file and commits should offer some assistance. it's also potentially worth splitting RNW 0.18 / Expo SDK 46 into a PR separate from Next 12.2, since they each have their own quirks. |
@michaelangrivera thank you for the showtime PR! I was not able to reproduce the error you posted above, was that on the solito monorepo? Or on this PR? @nandorojo that makes sense. Would you like me to close this PR? I can attempt to make 2 separate PRs for Expo and Next? |
@WillenOLeal Your upgrade PR helped me migrate my app to Expo 46 SDK but I had to do the following in order to get Renaimated 2/Moti to work on the web
I then had to make these changes to get EAS to build my project for iOS and Android:
|
@WillenOLeal NextJS 12.2.5 got released. The app starts but a bunch of console warnings are exposed, I think they are related to the underlying configuration |
@dohomi I wasn't able to reproduce the type error you got. I am on |
@WillenOLeal I found https://github.com/tamagui/starters where I am borrowing some additional config and types finally worked with latest versions. |
Hey Thank you @dohomi, for that repo! I was able to upgrade Nextjs to version 12.2.5 with no apparent server warnings. I pushed to this branch, left here as a reference. |
@WillenOLeal Your branch worked for me as well without hassle (after pulling hehehe). |
[withTM, withFonts, withImages, [withExpo, { projectRoot: __dirname }]], | ||
nextConfig | ||
) | ||
const transform = withPlugins([withTM, withFonts, withImages, withExpo]) |
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.
I wonder if we should keep the nextConfig
variable placed separately. This way we can get the JSDoc suggestions, and we can spread it onto here.
Also, is the projectRoot
not needed from Expo?
"solito": "latest" | ||
}, | ||
"resolutions": { | ||
"react-native-reanimated": "2.8.0" |
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.
I think resolutions should go in the root package.json
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.
In the packages/expo/package.json ""react-native-reanimated": "~2.9.1"
but the resolution is "react-native-reanimated": "2.8.0"
. Is there any specific reason why it's done like this ?
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.
I originally placed that resolution there, because on web the app would crash with this error: software-mansion/react-native-reanimated#3355 on version 2.9.x and up. I think it might be fixed as of the latest 3.0.x version of react-native-reanimated
FWIW, the custom fonts preview on vercel doesn't render the custom fonts, because the font files cannot be found. Don't know if that's related to this PR, but wanted to let you know in case you didn't see that. |
@SleeplessByte looks like an issue unrelated to this PR. I’ll take a look tomorrow |
@WillenOLeal could you fix the comments I left and then I'll review the PR locally? |
I wonder if there is a way to copy these changes into the other example monorepos without doing it manually...🫣 |
Thanks @WillenOLeal for this! Looks good to me. Merged! I added the following changes:
|
I am a noob to monorepos, and Next.js, but figured I would give my best attempt at upgrading the Solito monorepo. The project seems to run fine after the changes I made. The libs below were the ones that gave me the most trouble:
Upgrade Notes
next.js: 12.2.2
: Could not upgrade all the way to 12.2.4 kept getting 'Invalid next.config.js options detected' server waring vercel/next.js#39161react-native-reanimated: 2.8.0
: After upgrading to Expo SDK 46, with theexpo upgrade
command inside apps/expo, it installed react-native-reanimated v2.9.1. Native ran okay, but I kept getting this error on Web: software-mansion/react-native-reanimated#3355. So I put a yarn resolution onpackages/app
module for reanimated v2.8.0@types/react: 18.0.1
: Kept running into this next.js server error while using the latest version of this package, so I kept v18.0.1