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

docs(service-worker): updated Browser support for Service Worker #26408

Closed
wants to merge 1 commit into from

Conversation

nik72619c
Copy link
Contributor

@nik72619c nik72619c commented Oct 11, 2018

##Documentation regarding the changes for browser support of Service Worker

The docs status for the Prerequisites section of the angular service worker only specified that the only Chrome and Firefox provided the support for the same.

But according to the latest data from caniuse.com and MDN Web docs, many other browsers, along with their updated versions now support service workers.
Browsers like opera still do not provide the support.
Hence, provided the updated information regarding the same.

#Addition of a short code snippet

I also provided a short code snippet, illustrating how to detect the browser support for the service workers before rendering the code using the navigator object.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[x] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

According to the Angular.io Prerequisites section of the Service Worker Introduction, "Currently, the latest versions of Chrome and Firefox are supported."

Issue Number: 26308

What is the new behavior?

According to the Angular.io Prerequisites section of the Service Worker Introduction, "Currently, the latest versions of Chrome and Firefox are supported."
The new content added is the additional information of the browsers, along with their updated versions which now provide the support for the service worker, which were not mentioned earlier.

Also, a short, working code snippet was added for detecting the browser support for service worker, before running the code.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@nik72619c
Copy link
Contributor Author

#26408

created a pull request for issue 26408

Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

Thank, @nik72619c, for working on this 👍
I have left some suggestions/comments.

@@ -31,8 +31,18 @@ Installing the Angular service worker is as simple as including an `NgModule`. I

## Prerequisites

Your application must run in a web browser that supports service workers. Currently, the latest versions of Chrome and Firefox are supported. To learn about other browsers that are service worker ready, see the [Can I Use](http://caniuse.com/#feat=serviceworkers) page.

Your application must run in a web browser that supports service workers.Currently, all versions of Google Chrome and Firefox support service worker along with the latest versions of IE (17,18), Safari, UC Browser (Android version) and Samsung Internet.Versions like IE (11), IOS Safari(11.2) and Opera Mini yet do not provide the support. Although Edge, Firefox, Internet, Explorer and Opera have not been able to provide features like SafariNavigationPreloadManager, they provide basic support. To learn more about other browsers that are service worker ready, see the [Can I Use](http://caniuse.com/#feat=serviceworkers) page, [MDN Web Docs](https://developer.mozilla.org/en-US/docs/Web/API/Service_Worker_API) page.
Copy link
Member

Choose a reason for hiding this comment

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

Some typos that I noticed:

  • Missing space before "Currently, all" and "Versions like".
  • "support service worker" --> "support service workers".
  • "IOS Safari" --> "iOS Safari"
  • "Safari(11.2)" --> "Safari (11.2)"
  • "Internet, Explorer" --> "IE" (since this is how it is referred to in this doc (and most guide on angular.io))
  • "SafariNavigationPreloadManager" --> "NavigationPreloadManager" (but I would remove that whole sentence altogether, because it is confusing and not relevant to Angular's SW implementation atm)
  • "Can I Use page, MDN Web Docs page" --> "Can I Use page and MDN Web Docs page"

Some other suggestions:

  • Not all versions of Chrome and Firefox support custom elements. Please keep "latest" (or usementions specific versions).
  • Why "Google Chrome" instead of "Chrome"? (In this context, I think it is very clear what "Chrome" refers to.) If we were to mention vendors, we should do it for all browsers (but I don't think it makes sense).
  • IE does not support SW. Versions 17, 18 that you mention are versions of Edge (which is not IE - thank god 😛).
  • You don't have to mentions a specific version of IE (e.g. "IE (11)"), since IE is not getting any new versions and none of the existing versions support SW.
  • I would drop the "yet" from "yet do not provide the support" (because some of the mentioned browsers never will provide support.


}
</script>
```
Copy link
Member

Choose a reason for hiding this comment

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

This code snippet is redundant for @angular/service-worker, because users do not manually register their SW and Angular will already do the check under the hood.
Please remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the required changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made the changes ...

@nik72619c
Copy link
Contributor Author

Made the required changes !

Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

Thx fior working on this. I left a couple more suggestions 😇

@@ -31,7 +31,7 @@ Installing the Angular service worker is as simple as including an `NgModule`. I

## Prerequisites

Your application must run in a web browser that supports service workers. Currently, the latest versions of Chrome and Firefox are supported. To learn about other browsers that are service worker ready, see the [Can I Use](http://caniuse.com/#feat=serviceworkers) page.
Your application must run in a web browser that supports service workers. Currently, the latest versions of Chrome and Firefox support service workers along with Edge (17,18), Safari, UC Browser (Android version) and Samsung Internet. Versions like IE (11), iOS Safari (11.2) and Opera Mini do not provide the support. To learn more about other browsers that are service worker ready, see the [Can I Use](http://caniuse.com/#feat=serviceworkers) page and [MDN Web Docs](https://developer.mozilla.org/en-US/docs/Web/API/Service_Worker_API) page.
Copy link
Member

Choose a reason for hiding this comment

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

Some more suggestion:

  • Since you mention Opera Mini, I think it is a good idea to also mention that Opera does support SW (to avoid confusion).
  • You can drop the version after IE (11), since no version of IE will ever support SW.
  • According to Can I Use, the latest version of iOS Safari does support SW.
  • Change http://caniuse.com to https://caniuse.com.

Overall, I would change this paragraph to something like:

Your application must run in a web browser that supports service workers. Currently, service workers are supported in the latest versions of Chrome, Firefox, Edge, Safari, Opera, UC Browser (Android version) and Samsung Internet. Browsers like IE and Opera Mini do not provide the support. To learn more about other browsers that are service worker ready, see the [Can I Use](https://caniuse.com/#feat=serviceworkers) page and [MDN docs](https://developer.mozilla.org/en-US/docs/Web/API/Service_Worker_API).

@nik72619c
Copy link
Contributor Author

I made the suggested changes...

@gkalpak gkalpak added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews comp: docs target: patch This PR is targeted for the next patch release area: service-worker Issues related to the @angular/service-worker package labels Oct 17, 2018
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

Awesome ❤️ (Thx for staying with me 😁)
Can you please squash the commits into one and change the commit message to:

docs(service-worker): update browser support for Service Worker

@nik72619c
Copy link
Contributor Author

done!

@gkalpak
Copy link
Member

gkalpak commented Oct 19, 2018

Thx for making the change, but there seems to be a space after docs, which makes our linter unhappy (and CI fails).
Can you please remove the space (docs (service-worker): ... --> docs(service-worker): ...)?

@nik72619c
Copy link
Contributor Author

nik72619c commented Oct 19, 2018

changed it! Now does it look fine ?

@gkalpak gkalpak changed the title docs: Updated Browser support for Service Worker docs(service-worker): updated Browser support for Service Worker Oct 19, 2018
@gkalpak gkalpak added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Oct 19, 2018
@ngbot ngbot bot added this to the needsTriage milestone Oct 19, 2018
@gkalpak
Copy link
Member

gkalpak commented Oct 19, 2018

👌
CI is happy. I'm happy. Let's get this merged 😛

@gkalpak
Copy link
Member

gkalpak commented Oct 19, 2018

Thx again for working on this, @nik72619c 👍

@nik72619c
Copy link
Contributor Author

Thank you for guiding me out @gkalpak !

@alxhub alxhub closed this in 07fc4c2 Oct 19, 2018
sculove pushed a commit to sculove/angular that referenced this pull request Nov 2, 2018
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: service-worker Issues related to the @angular/service-worker package cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants