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

Upgraded Expo to version 46 and Next.js to version 12.2.x #120

Merged
merged 5 commits into from Sep 8, 2022

Conversation

WillenOLeal
Copy link
Contributor

@WillenOLeal WillenOLeal commented Aug 7, 2022

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#39161

react-native-reanimated: 2.8.0: After upgrading to Expo SDK 46, with the expo 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 on packages/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

@vercel
Copy link

vercel bot commented Aug 7, 2022

Someone is attempting to deploy a commit to a Personal Account owned by @nandorojo on Vercel.

@nandorojo first needs to authorize it.

@vercel
Copy link

vercel bot commented Aug 7, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
solito ✅ Ready (Inspect) Visit Preview Aug 30, 2022 at 10:06PM (UTC)
solito-app ✅ Ready (Inspect) Visit Preview Aug 30, 2022 at 10:06PM (UTC)
with-custom-fonts ✅ Ready (Inspect) Visit Preview Aug 30, 2022 at 10:06PM (UTC)

@nandorojo
Copy link
Owner

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.

@michaelangeloio
Copy link

Not sure if Next 12.2 is compatible, I was getting this weird error

error - Error: "Document.getInitialProps()" should resolve to an object with a "html" prop set with a valid html string
    at loadDocumentInitialProps (/Users/angelo/developer/mileaday/node_modules/next/dist/server/render.js:774:23)
    at async renderDocument (/Users/angelo/developer/mileaday/node_modules/next/dist/server/render.js:856:43)
    at async Object.renderToHTML (/Users/angelo/developer/mileaday/node_modules/next/dist/server/render.js:893:28)
    at async doRender (/Users/angelo/developer/mileaday/node_modules/next/dist/server/base-server.js:912:38)
    at async cacheEntry.responseCache.get.isManualRevalidate.isManualRevalidate (/Users/angelo/developer/mileaday/node_modules/next/dist/server/base-server.js:1017:28)
    at async /Users/angelo/developer/mileaday/node_modules/next/dist/server/response-cache.js:69:36 {
  page: '/'
}

Which seemed to be affecting the custom default export behavior in _document.tsx. Downgrading from 12.2 to 12.1 solved the issue I had locally.

@nandorojo
Copy link
Owner

i believe showtime faced this, could you reference their repo for a solution?

@michaelangeloio
Copy link

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.

@nandorojo
Copy link
Owner

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.

@WillenOLeal
Copy link
Contributor Author

@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?

@popejs
Copy link

popejs commented Aug 12, 2022

@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

  • next.config.js

    reactStrictMode: false
    
  • app -> package.json

    "resolutions": {
      "react-native-reanimated": "2.8.0"
    },
    

I then had to make these changes to get EAS to build my project for iOS and Android:

  • app -> expo -> package.json

    "workspaces": {
        "nohoist": [
          "react-native",
          "**/react-native-codegen"
        ]
      }
    

@dohomi
Copy link

dohomi commented Aug 14, 2022

I ran into the upgrade process myself and realised that there seems to be mismatch with the new typings. How did you guys get around this?
Screenshot 2022-08-14 at 12 44 09

@dohomi
Copy link

dohomi commented Aug 14, 2022

@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

@WillenOLeal
Copy link
Contributor Author

@dohomi I wasn't able to reproduce the type error you got. I am on @types/react: 18.0.17 and @types/react-native: 0.69.5. Also yeah I tried upgrading to 12.2.5, but kept getting a bunch of warnings, regarding the next.config.js file invalid options, still trying to figure out how to resolve them. I checked the showtime-xyz/showtime-frontend project, they are on 12.2.3-canary.2 as of writing, this version seems to work okay for now.

@dohomi
Copy link

dohomi commented Aug 19, 2022

@WillenOLeal I found https://github.com/tamagui/starters where I am borrowing some additional config and types finally worked with latest versions.

@WillenOLeal
Copy link
Contributor Author

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.

@SleeplessByte
Copy link

SleeplessByte commented Aug 23, 2022

@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])
Copy link
Owner

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"
Copy link
Owner

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

Copy link

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 ?

Copy link
Contributor Author

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

@SleeplessByte
Copy link

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.

@nandorojo
Copy link
Owner

@SleeplessByte looks like an issue unrelated to this PR. I’ll take a look tomorrow

@nandorojo
Copy link
Owner

@WillenOLeal could you fix the comments I left and then I'll review the PR locally?

@nandorojo
Copy link
Owner

I wonder if there is a way to copy these changes into the other example monorepos without doing it manually...🫣

@nandorojo nandorojo merged commit 74aba92 into nandorojo:master Sep 8, 2022
@nandorojo
Copy link
Owner

nandorojo commented Sep 8, 2022

Thanks @WillenOLeal for this! Looks good to me. Merged!

I added the following changes:

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 this pull request may close these issues.

None yet

8 participants