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

fix(android): use expo-platform over user-agent #46

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

szymonrybczak
Copy link
Collaborator

For some reason on Android user-agent doesn't contain React or Expo keyword.

@szymonrybczak szymonrybczak changed the title fix: use expo-platform over user-agent fix(android): use expo-platform over user-agent Apr 13, 2024
@szymonrybczak
Copy link
Collaborator Author

szymonrybczak commented Apr 13, 2024

@natew could you verify if that works in your setup? my is still breaking because of some strange react-native-screens imports problem (even after applying patch).

@natew
Copy link
Collaborator

natew commented Apr 14, 2024

Breaking on android? I think we patch just one of the files, does android have a specific platform file? That's one thing we need to get better support of in general.

@szymonrybczak
Copy link
Collaborator Author

szymonrybczak commented Apr 14, 2024

Hm, yea - after applying patch properly, Rollup doesn't give the error. But esbuild only resolves the first extension it found, so in our case it cause another problem - when we're fetching bundle for Android and we have:

    resolveExtensions: [
      '.ios.js',
      '.android.js',
    ]

it will pick Platform.ios.js and in react-native.js we'll have OS: "ios" which is the reason why it doesn't work. I guess we need to pass proper extension based on ?platform query param

@@ -529,12 +529,18 @@ export const create = async (optionsIn: VXRNConfig) => {
// TODO move these to router.get():
app.use(
defineEventHandler(async ({ node: { req } }) => {
if (!req.headers['user-agent']?.match(/Expo|React/)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

btw wont this break now for RN vanilla? we may need both or something.

Copy link
Collaborator Author

@szymonrybczak szymonrybczak Apr 16, 2024

Choose a reason for hiding this comment

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

no, because bare RN at beginning makes request to:

 → /status
 → /index.bundle

so this change won't affect it

@szymonrybczak szymonrybczak force-pushed the fix/use-expo-platform-over-user-agent branch from 6372fd0 to 95fd88a Compare April 16, 2024 10:25
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

2 participants