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

Fix isPush support on safari in an iframe #548

Merged
merged 1 commit into from
Sep 30, 2019

Conversation

jkasten2
Copy link
Member

@jkasten2 jkasten2 commented Sep 29, 2019

  • Unfortunately window.safari is not defined on Safari if in an iframe context
  • Due to this there is no other way than to have fallback logic to
    check navigator properties if we are on macOS Safari.

This change is Reviewable

* Unfortunately window.safari is not defined on Safari if in an iframe context
* Due to this there is no other way than to have fallback logic to
   check navigator properties if we are on macOS Safari.
@jkasten2 jkasten2 requested a review from itrush September 29, 2019 07:59
Copy link
Contributor

@itrush itrush left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jkasten2)


src/utils/BrowserSupportsPush.ts, line 36 at r1 (raw file):

//   HTTP - navigator.serviceWorker == "undefined"

I don't see this one checked anywhere, did you miss it or checking for PushSubscriptionOptions type is enough?

Copy link
Member Author

@jkasten2 jkasten2 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @itrush)


src/utils/BrowserSupportsPush.ts, line 36 at r1 (raw file):

Previously, itrush (Iryna Trush) wrote…
//   HTTP - navigator.serviceWorker == "undefined"

I don't see this one checked anywhere, did you miss it or checking for PushSubscriptionOptions type is enough?

Just wanted to point out that navigator.serviceWorker is undefined in Chrome in a HTTP context to note it that it can't be used to detect the browser.

Copy link
Contributor

@itrush itrush left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@jkasten2 jkasten2 merged commit 7f6973b into master Sep 30, 2019
@jkasten2 jkasten2 deleted the is_push_supported_safari_iframe branch September 30, 2019 18:56
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