-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat(kno-6559): set up @knocklabs/expo package #243
feat(kno-6559): set up @knocklabs/expo package #243
Conversation
🦋 Changeset detectedLatest commit: 30f6c35 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
…o-is-not-a-required-dependency
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.
one q
packages/expo/package.json
Outdated
@@ -0,0 +1,80 @@ | |||
{ | |||
"name": "@knocklabs/expo", | |||
"version": "0.3.1", |
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.
Just to confirm, is this right for the initial version? Do we not want to start at 0.1.0?
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.
We can definitely start at v0.1.0. I wasn’t sure what’s preferable: restarting version numbering at 0 since this is a new package, or starting with the same version number as our RN SDK since the new package is essentially a fork.
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’ll set the version
attribute here to 0.0.0
, so that when changesets bumps the version code, the first release will be 0.1.0
.
Summary
This PR sets up a new package
@knocklabs/expo
for use with React Native + Expo projects, and moves the existingKnockExpoPushNotificationProvider
plus all required Expo dependencies to@knocklabs/expo
.Background
At the moment,
@knocklabs/react-native
’spackage.json
includes all Expo-related dependencies as optional peer dependencies:javascript/packages/react-native/package.json
Lines 43 to 64 in f902b99
However,
KnockExpoPushNotificationProvider.tsx
imports these dependencies using static imports:javascript/packages/react-native/src/modules/push/KnockExpoPushNotificationProvider.tsx
Lines 7 to 9 in f902b99
This causes build errors when Metro (the bundler used by React Native) bundles an application that depends on
@knocklabs/react-native
but does not include Expo dependencies.It’s possible to refactor
KnockExpoPushNotificationProvider
to import Expo modules dynamically usingrequire
. Support for loading optional dependencies viarequire
was added to Metro v0.59.0 by facebook/metro#511.However, optional dependencies in React Native are subject to some longstanding issues, e.g. facebook/metro#836 and facebook/metro#1256. (And sure enough, during my testing locally, I encountered these issues.) Even if we load Expo modules using
require
and avoid these bugs, it’s highly likely future refactoring of the code would cause the build errors to occur again. For this reason, we feel it’s safer to migrate all code in@knocklabs/react-native
that relies upon Expo to a new Expo-specific SDK.Testing
I tested everything locally and confirmed the following:
@knocklabs/react-native
and@knocklabs/expo
successfully receive notifications viaKnockFeedProvider
KnockExpoPushNotificationProvider
after moving it to the@knocklabs/expo
package@knocklabs/react-native
have been fixed