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

Feat/safari 16 #930

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

Feat/safari 16 #930

wants to merge 19 commits into from

Conversation

rgomezp
Copy link
Contributor

@rgomezp rgomezp commented Nov 18, 2022

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

  • WebSDK
  • Backend
  • Dashboard

Validation

Tests

Info

Checklist

  • All the automated tests pass or I explained why that is not possible
  • I have personally tested this on my machine or explained why that is not possible
  • I have included test coverage for these changes or explained why they are not needed

Programming Checklist
Interfaces:

  • Don't use default export
  • New interfaces are in model files

Functions:

  • Don't use default export
  • All function signatures have return types
  • Helpers should not access any data but rather be given the data to operate on.

Typescript:

  • No Typescript warnings
  • Avoid silencing null/undefined warnings with the exclamation point

Other:

  • Iteration: refrain from using elem of array syntax. Prefer forEach or use map
  • Avoid using global OneSignal accessor for context if possible. Instead, we can pass it to function/constructor so that we don't call OneSignal.context

Screenshots

Info

Screen Shot 2023-03-06 at 2 19 21 PM
Currently Safari 16 does not display the custom icon. This is an upstream limitation.

Checklist

  • I have included screenshots/recordings of the intended results or explained why they are not needed


This change is Reviewable

@rgomezp rgomezp marked this pull request as ready for review January 20, 2023 19:55
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
jkasten2 and others added 8 commits March 4, 2023 01:00
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.
@@ -214,4 +214,13 @@ export default class MainHelper {
const subscription = await OneSignal.database.getSubscription();
return subscription.deviceId || undefined;
}

static async getCurrentPushToken(): Promise<string | undefined> {
Copy link
Member

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.

@jkasten2 jkasten2 mentioned this pull request Mar 9, 2023
13 tasks
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