Skip to content

feat(clerk-expo): Add expo data to request headers - #2528

Merged
thiskevinwang merged 6 commits into
mainfrom
kevin/core-1401-improve-detection-for-mobile-apps-using-clerk
Jan 10, 2024
Merged

feat(clerk-expo): Add expo data to request headers#2528
thiskevinwang merged 6 commits into
mainfrom
kevin/core-1401-improve-detection-for-mobile-apps-using-clerk

Conversation

@thiskevinwang

@thiskevinwang thiskevinwang commented Jan 9, 2024

Copy link
Copy Markdown
Member

Description

What
Send along x-expo-execution-environment and x-expo-native-application-version headers to help identify the state of a mobile app, calling Clerk.

Why
We want to be able to easily determine if a mobile app is in any of "local development", "test flight", "app store/google play" (aka: dev/staging/production, respectively)

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

Packages affected

  • @clerk/backend
  • @clerk/chrome-extension
  • @clerk/clerk-js
  • @clerk/clerk-expo
  • @clerk/fastify
  • gatsby-plugin-clerk
  • @clerk/localizations
  • @clerk/nextjs
  • @clerk/clerk-react
  • @clerk/remix
  • @clerk/clerk-sdk-node
  • @clerk/shared
  • @clerk/themes
  • @clerk/types
  • build/tooling/chore

@changeset-bot

changeset-bot Bot commented Jan 9, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 1bf9c09

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@clerk/clerk-expo Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Comment thread packages/expo/src/singleton.ts Outdated
Comment on lines +60 to +65
(requestInit.headers as Headers).set('x-expo-application-id', Application.applicationId ?? NULL_VALUE);
(requestInit.headers as Headers).set('x-expo-application-name', Application.applicationName ?? NULL_VALUE);
(requestInit.headers as Headers).set(
'x-expo-native-application-version',
Application.nativeApplicationVersion ?? NULL_VALUE,
);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What insight might we gain from the app name / id?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

💭 app name == Expo Go is roughly equivalent to request.origin == http://localhost:3000

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

More generally, my thinking here falls mostly under "I don't know what I don't know" regarding insights. But presence of this data should at least enable gaining any insight at all!

@jescalan jescalan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks good to me, but I'm not sure I understand the context behind why it's being done. The PR description is just a screenshot of the result, the name of the branch honestly is the closest to context that we have, but even checking the related linear issue pulled from the branch name, there isn't really a lot of context about why this is important or where it came from.

To be clear, I trust that it's for a good reason, just some feedback on how to make PRs like this easier to understand and review for folks down the line 😁

@thiskevinwang
thiskevinwang requested a review from brkalow January 10, 2024 14:40
@thiskevinwang

Copy link
Copy Markdown
Member Author

@brkalow I reduced the number of headers sent to 2.

@jescalan I updated the description.

Comment thread .changeset/sixty-rats-bathe.md Outdated

@brkalow brkalow left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍

@thiskevinwang
thiskevinwang added this pull request to the merge queue Jan 10, 2024
Merged via the queue into main with commit 3399635 Jan 10, 2024
@thiskevinwang
thiskevinwang deleted the kevin/core-1401-improve-detection-for-mobile-apps-using-clerk branch January 10, 2024 17:54
thiskevinwang added a commit that referenced this pull request Jan 10, 2024
github-merge-queue Bot pushed a commit that referenced this pull request Jan 10, 2024
* chore: cherrypick #2528

* chore(repo): Reset package-lock to release/v4 && npm i
@auropsoft

auropsoft commented Feb 24, 2024

Copy link
Copy Markdown

This change causes issues with the use of clerk/clerk-expo with expo start --web.
The preflight requests get blocked by clerk with the standard message in the browser:
...blocked by CORS policy: Request header field x-expo-native-application-version is not allowed by Access-Control-Allow-Headers in preflight response

Access to fetch at 'https://gorgeous-swift-17.clerk.accounts.dev/v1/environment?_clerk_js_version=4.69.1&_method=PATCH&_is_native=1&__dev_session=eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJkZXYiOiJkdmJfMmNwRFhxOVpWNFpZQzhLVk5lSXBSeklmNHhWIn0.awSF7yCL7AB4HTf1PjBpzEi-uIINoVX8n6u6pkBq2lIQ91MXHFdISlEXLX_G28DOGAYqZReu5NJdKn-MejXBXAdy99ohZ4hxhAMwz5bnRUtnZ94OAFEY2jBBHfsTW9YjNipWjOlTbE1FHJ5QM06hS3TiJaVUx9ZqBOHRGcGIj9_lbHdEfSjrX2lULEiV59ggqWEPl7FOlbDGLdyhqv3CnwxmH65EY-TT2G-12Fues1KA3i2zyOppNONC7cNz1eVuFEnQfavIeg31-vgCqN-SjbEG2-QesQvozE_ABMZxvy_ij4-SP7uRjUCYwLoAu5Z05MMY52ULroVojTbI1CnZkA' from origin 'http://localhost:8081' has been blocked by CORS policy: Request header field x-expo-native-application-version is not allowed by Access-Control-Allow-Headers in preflight response.

Adding a bit of code to remove those headers for web platform fixes the issue:

    useEffect(() => {
        if(Platform.OS === 'web') {
            clerk.__unstable__onBeforeRequest(async (requestInit: RequestInit) => {
                (requestInit.headers as Headers).delete("x-expo-execution-environment");
                (requestInit.headers as Headers).delete("x-expo-native-application-version");
            },[]);
        }
    })

I assume that web, being the underused platform for RN, wasn't specifically tested, but i'd love to be able to run it with the same code as the other platforms.

Does this have to be a seperate report?
This feels like a server-side issue, not so much a package issue. One would assume the statistics would want to be gathered either way from the description.

@thiskevinwang

Copy link
Copy Markdown
Member Author

Hey @auropsoft,

Does this have to be a seperate report?
This feels like a server-side issue, not so much a package issue. One would assume the statistics would want to be gathered either way from the description.

Currently, the @clerk/expo SDK only supports mobile app output, and not web, so this flow was not tested. This is not necessarily a bug right now, but a helpful breadcrumb for us nonetheless (thanks for using the Expo SDK 😄 ).

Does this sound reasonable?


Tip

You can upvote the addition of Clerk supporting web output here!

thiskevinwang added a commit that referenced this pull request May 10, 2024
* feat(clerk-expo): Remove headers and dev dependencies
- this reverts #2528
@clerk-cookie clerk-cookie mentioned this pull request May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants