Skip to content

Conversation

@sync
Copy link

@sync sync commented Dec 7, 2025

Description

We have started relying on knock and are faced with some "issues" with the current sdk, wondering if you would be interested in applying some of those fixes?

Checklist

  • Tests have been added for new features or major refactors to existing features.

@changeset-bot
Copy link

changeset-bot bot commented Dec 7, 2025

⚠️ No Changeset found

Latest commit: 4ed8dea

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@vercel
Copy link

vercel bot commented Dec 7, 2025

@sync is attempting to deploy a commit to the Knock Team on Vercel.

A member of the Team first needs to authorize it.

};

fetchExpoTokenIfNeeded();
}, []);
Copy link
Author

Choose a reason for hiding this comment

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

address issue when you want to manually call registerPushTokenToChannel and your user already has granted those permissions, if not the expoPushToken from the provider will be undefined till you call registerForPushNotifications

"interacted",
);
notificationTappedHandler(response);
}
Copy link
Author

Choose a reason for hiding this comment

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

Image

It's how expo recommend handling the initial push notifications: https://docs.expo.dev/versions/latest/sdk/notifications/#handle-push-notifications-with-navigation

registerPushTokenToChannel(
token: string,
channelId: string,
locale?: string,
Copy link
Author

Choose a reason for hiding this comment

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

the locale in our app can be overridden (not system wide) and at the moment there is no way to tell knock about the custom user's locale

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks reasonable.

Copy link
Author

Choose a reason for hiding this comment

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

let me know if you want me to split this pr into multiple

@sync sync changed the title Add possibility to provide a local when registering push token, make sure to not miss the last notification interaction and get expo push token if has permissions on boot Add possibility to provide a locale when registering push token, make sure to not miss the last notification interaction and get expo push token if has permissions on boot Dec 8, 2025
@sync sync requested a review from a team as a code owner December 10, 2025 22:04
@sync sync requested review from MikeCarbone and thomaswhyyou and removed request for a team December 10, 2025 22:04
"expo": "~53.0.22",
"expo-constants": "~17.1.7",
"expo-device": "^7.1.4",
"expo-network": "^8.0.0",
Copy link
Author

Choose a reason for hiding this comment

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

it's probably a breaking change

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.

2 participants