-
-
Notifications
You must be signed in to change notification settings - Fork 428
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
chore(expo) : Update to Expo 50 #795
Conversation
.npmrc
Outdated
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.
can we nuke this file all together now? I think strict-peer-dependencies was also set for expo reasons
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.
You can on the native side! We've been working hard to get rid of all implicit dependency references in both /android
and /ios
folders.
That means things like expo run android|ios
are working as intended, without node-linker=hoisted
.
Unfortunately, there are a few left-over issues, e.g. eas update
won't work properly with this yet. Working hard to resolve them before SDK 50 stable is released.
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.
👀 not needed anymore?
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'm not sure why the reactNativeVersion
and expoPackageVersion
is being used, but it's not in our default template files 😄 (code)
apps/expo/index.js
Outdated
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.
this shouldn't be needed afaik?
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.
Exactly, you can, but you don't need to. The only reason why I did this is to let Turborepo determine if the Metro cache can be reused.
This was mostly due to the fact that there was no environment variable support, and therefore Metro did reuse the cache incorrectly when this was changed.
But, now that we have @expo/env
and proper environment variables support, this shouldn't be necessary anymore.
@@ -2,10 +2,10 @@ | |||
"name": "@acme/expo", | |||
"version": "0.1.0", | |||
"private": true, | |||
"main": "expo-router/entry", |
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.
this should work according to their docs? did they go back?
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.
This should work fine. We did discover an issue in expo/AppEntry
where we try to import ../../App
, which doesn't work for .pnpm/expo@.../node_modules/expo/AppEntry.js
.
We know about that issue, and will fix it as soon as we can!
apps/expo/tsconfig.json
Outdated
@@ -1,5 +1,5 @@ | |||
{ | |||
"extends": "@acme/tsconfig/base.json", | |||
"extends": ["@acme/tsconfig/base.json", "expo/tsconfig.base"], |
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.
change order here?
Are there still blockers now thst 50 is properly released? |
Hi, does this intend to be reopened? @VaniaPopovic ? Or maybe we can create a new PR |
.npmrc
Outdated
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.
can we remove this entirely? idk when the strict-peer-dependency setting was introduced but ideally we wouldn't need it anymore
apps/expo/index.js
Outdated
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 example on expo uses the package.json field, should be fine to keep that right? https://github.com/expo/expo/blob/main/templates/expo-template-tabs/package.json#L3
idk what the main reason was for android not to work with that but if their pnpm support has gotten better i assume this issue might be fixed?
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.
saw this comment, idk how much progress they'vem ade since? https://github.com/t3-oss/create-t3-turbo/pull/795/files#r1432898627
Closes #850
Closes #849
Currently there's an issue with NativeWind failing to import react-native-css-interop/jsx-runtime that it depends on. Will raise an issue in Nativewind's repo and update this PR with the status.
Nativewind issue to track : nativewind/nativewind#701