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/safari 16 #930
Open
rgomezp
wants to merge
19
commits into
main
Choose a base branch
from
feat/safari-16
base: main
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Feat/safari 16 #930
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rgomezp
force-pushed
the
feat/safari-16
branch
from
January 20, 2023 19:54
2eacfcc
to
3ff9f12
Compare
Motivation: to allow use in other files
Motivation: to easily detect old Safari browsers not using the PushAPI
Motivation: to maintain support for old Safari browsers, we should continue subscribing Safari users via the current method.
Motivation: VAPID Safari devices should send 16. Cleanup: remove `isSafari` which already exists on `Environment`
Motivation: move away from FCM naming to Vapid naming
Motivation: push token should update automatically on subscription change
Motivation: to prevent duplicate subscriptions, we need to unsubscribe the legacy Safari subscription if it is present after successfully subscribing down the VAPID path.
Motivation: 16 was already reserved
This is a requirment to support WebPush on iOS 16.4 devices. See Apple's blog post for more details: https://webkit.org/blog/13878/web-push-for-web-apps-on-ios-and-ipados/
Disabling this by default as it doesn't work with the manifest.json. Only commenting it out as this is still a test case we want to support and went to continue making it easy to test.
…ios_push [Sandbox] Add web app manifest for ios push testing
Adding a catch here ensures we still subscribe for a vapid push token. Also this catch fixes an issue where on_session calls would no longer happen
Was not able to reproduce a specific scenario where this would result in the wrong registration for Safari this code definitely would use the old registration for Safari when it should use VAPID. Also added logging so we can spot any issue with this in the future.
The app_id is require to ensure the correct token is subscribed on OneSignal's backend, as the browser could be subscribed to more than one site with the same safari bundle id.
jkasten2
reviewed
Mar 9, 2023
@@ -214,4 +214,13 @@ export default class MainHelper { | |||
const subscription = await OneSignal.database.getSubscription(); | |||
return subscription.deviceId || undefined; | |||
} | |||
|
|||
static async getCurrentPushToken(): Promise<string | undefined> { |
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.
This function was added in this PR but I don't see it being used anywhere.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
1 Line Summary
Adds support for Safari 16 (VAPID).
Details
Safari 16 is introducing the standard PushAPI that Chrome, Firefox, and other browsers use today.
Safari 16 still supports it’s proprietary “Safari Push Notifications” on macOS, but the standard API is preferred since it supports more features. iOS/iPadOS 16.4 also supports the standard PushAPI. Apple will almost certainly drop the old API for macOS, but it may be a number of years before then so we will continue supporting it. We still want to continue supporting macOS 12.x and older, since it requires end users update to macOS 13 to get it.
Device Type
Introducing
device_type: 17
for W3C version of Safari. Since the two Safari push types are fundamentally different, a new device_type value would clearly separate them and would keep things consistent in our system.Data Purge Problem:
Safari has a 7 day window where after the user doesn’t visit the site, it will purge all data like IndexedDB and localStorage. We introduce a way to detect whether the user was previously subscribed to legacy notifications and avoid creating a second subscription.
iOS Support
With the introduction of web push for Safari on iOS, these changes will also work for that feature. However, it will require the site to host a manifest.json file. This PR updates our sandbox environment with that file.
Systems Affected
Validation
Tests
Info
Checklist
Programming Checklist
Interfaces:
Functions:
Typescript:
Other:
elem of array
syntax. PreferforEach
or usemap
context
if possible. Instead, we can pass it to function/constructor so that we don't callOneSignal.context
Screenshots
Info
Currently Safari 16 does not display the custom icon. This is an upstream limitation.
Checklist
This change is