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

chore(expo) : Update to Expo 50 #795

Merged
merged 19 commits into from
Jan 28, 2024
Merged

chore(expo) : Update to Expo 50 #795

merged 19 commits into from
Jan 28, 2024

Conversation

VaniaPopovic
Copy link
Contributor

@VaniaPopovic VaniaPopovic commented Dec 14, 2023

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.
image

Nativewind issue to track : nativewind/nativewind#701

.npmrc Outdated
Copy link
Member

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

Copy link
Contributor

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👀 not needed anymore?

Copy link
Contributor

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)

Copy link
Member

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?

Copy link
Contributor

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",
Copy link
Member

@juliusmarminge juliusmarminge Dec 14, 2023

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?

Copy link
Contributor

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!

@@ -1,5 +1,5 @@
{
"extends": "@acme/tsconfig/base.json",
"extends": ["@acme/tsconfig/base.json", "expo/tsconfig.base"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change order here?

@juliusmarminge
Copy link
Member

Are there still blockers now thst 50 is properly released?

@dBianchii
Copy link
Contributor

Hi, does this intend to be reopened? @VaniaPopovic ? Or maybe we can create a new PR

@VaniaPopovic VaniaPopovic reopened this Jan 27, 2024
.npmrc Outdated
Copy link
Member

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

Copy link
Member

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?

Copy link
Member

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

apps/expo/src/app/_layout.tsx Outdated Show resolved Hide resolved
apps/expo/src/app/index.tsx Outdated Show resolved Hide resolved
apps/expo/tailwind.config.ts Show resolved Hide resolved
@juliusmarminge juliusmarminge marked this pull request as ready for review January 28, 2024 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants