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(service-worker): avoid uncaught rejection warning when registration fails #30876

Closed

Conversation

Projects
None yet
3 participants
@H--o-l
Copy link
Contributor

commented Jun 5, 2019

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
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

After using ServiceWorkerModule.register('/ngsw-worker.js'),, if you have an unstable internet connection, navigator.serviceWorker.register('/ngsw-worker.js') call can fail and your application can crash.
I think that the error can't be caught on the user side.

What is the new behavior?

If navigator.serviceWorker.register('/ngsw-worker.js') fail, there is no uncaught error anymore.

Does this PR introduce a breaking change?

  • Yes
  • No (I'm not completely sure)

Other information

N/A

@H--o-l H--o-l requested a review from angular/fw-service-worker as a code owner Jun 5, 2019

@googlebot

This comment has been minimized.

Copy link

commented Jun 5, 2019

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no label Jun 5, 2019

@H--o-l

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

I signed it!

@googlebot

This comment has been minimized.

Copy link

commented Jun 5, 2019

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Jun 5, 2019

@H--o-l H--o-l force-pushed the H--o-l:fix/do_not_crash_if_serviceWorker_fail branch from 2e8642e to a0ebcf5 Jun 5, 2019

readyToRegister$.pipe(take(1)).subscribe(
() => navigator.serviceWorker.register(script, {scope: options.scope}));
() => navigator.serviceWorker.register(script, {scope: options.scope}).catch(() => {}));

This comment has been minimized.

Copy link
@gkalpak

gkalpak Jun 5, 2019

Member

I don't think it is a good idea to swallow that error. If the SW fails to register (for whatever reason), the developer should know.
If you want to avoid that the uncaught rejection error, maybe we could explicitly pass it to console.error() inside the catch()? WDYT?

This comment has been minimized.

Copy link
@H--o-l

H--o-l Jun 6, 2019

Author Contributor

Hey @gkalpak, thanks for the feedback.
It's a good idea, here is the diff I just push in my commit:

--- a/packages/service-worker/src/module.ts
+++ b/packages/service-worker/src/module.ts
@@ -118,7 +118,9 @@ export function ngswAppInitializer(
     // Don't return anything to avoid blocking the application until the SW is registered.
     // Catch and ignore the error if SW registration fails.
     readyToRegister$.pipe(take(1)).subscribe(
-        () => navigator.serviceWorker.register(script, {scope: options.scope}).catch(() => {}));
+        () =>
+            navigator.serviceWorker.register(script, {scope: options.scope})
+                .catch(err => console.error('NGSW - service worker register failed with: ', err)));

PS: Would you be able to help me fixing the CI tests ?
If you think this fix is a good idea of course.

This comment has been minimized.

Copy link
@gkalpak

gkalpak Jun 6, 2019

Member

The failing tests on CI are due to the fact that we are mocking serviceWorker.register() here, but not returning a promise (which was not necessary before). You need to change the above line to something like:

-beforeEach(() => swRegisterSpy = spyOn(navigator.serviceWorker, 'register'));
+beforeEach(() => swRegisterSpy = spyOn(navigator.serviceWorker, 'register').and.returnValue(Promise.resolve()));

@ngbot ngbot bot added this to the needsTriage milestone Jun 5, 2019

@H--o-l H--o-l force-pushed the H--o-l:fix/do_not_crash_if_serviceWorker_fail branch from a0ebcf5 to 589c6ab Jun 6, 2019

@gkalpak
Copy link
Member

left a comment

Thx, @H--o-l. I left a couple of minor comments, but otherwise lgtm.
I also suggest some changes to the commit message:

  • Change the subject to fix(service-worker): avoid uncaught rejection warning when registration fails
  • Typo: "can failed" --> "can fail"
  • Typo: " a `Uncaught" --> "an `Uncaught"
  • Change the last sentence to describe the new behavior (i.e. logging the error and not ignoring the error).
// Don't return anything to avoid blocking the application until the SW is registered or
// causing a crash if the SW registration fails.
// Don't return anything to avoid blocking the application until the SW is registered.
// Catch and ignore the error if SW registration fails.

This comment has been minimized.

Copy link
@gkalpak

gkalpak Jun 6, 2019

Member

We are not really ignoring it any more. Maybe change to Catch and log the error if SW registration fails to avoid uncaught rejection warning?

() => navigator.serviceWorker.register(script, {scope: options.scope}));
() =>
navigator.serviceWorker.register(script, {scope: options.scope})
.catch(err => console.error('NGSW - service worker register failed with: ', err)));

This comment has been minimized.

Copy link
@gkalpak

gkalpak Jun 6, 2019

Member

I would change it to just Service worker registration failed with:.

@H--o-l H--o-l force-pushed the H--o-l:fix/do_not_crash_if_serviceWorker_fail branch from 589c6ab to 1d53c41 Jun 6, 2019

@H--o-l H--o-l changed the title fix(service-worker): don't crash if registration failed fix(service-worker): avoid uncaught rejection warning when registration fails Jun 6, 2019

@H--o-l

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

@gkalpak, CI looks good, the fix was exactly what you proposed.
I also update the commit and commit message with changes you suggested.

Thanks for your help !

@gkalpak
Copy link
Member

left a comment

👍

Looking at the test fix again, I think it is a good idea to add a test for the new behavior (in module_spec.ts). It could go like this:

it('catches and a logs registration errors', async() => {
  const consoleErrorSpy = spyOn(console.error);
  swRegisterSpy.and.returnValue(Promise.reject('no reason'));

  await configTestBed({enabled: true, scope: 'foo'});
  expect(consoleErrorSpy).toHaveBeenCalledWith('Service worker registration failed with:' , 'no reason');
});
fix(service-worker): avoid uncaught rejection warning when registrati…
…on fails

Service worker API `navigator.serviceWorker.register` can fail in multiple ways.
For example, in Chrome, with an unstable network connection you can have the
following error: `An unknown error occurred when fetching the script.`

In the current state, it creates an `Uncaught (in promise) TypeError:` style of
error, which cannot be caught by the user on his own.

I think it's better to log the error over raising an error that cannot be
caught.

@H--o-l H--o-l force-pushed the H--o-l:fix/do_not_crash_if_serviceWorker_fail branch from 1d53c41 to 31f7764 Jun 7, 2019

@H--o-l

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

Hey @gkalpak, thanks for the idea, and the test I should have wrote myself from the beginning.
I add it to the commit, inside the describe('register()', () => { test section.
CI looks to like it 💪

@gkalpak

gkalpak approved these changes Jun 7, 2019

Copy link
Member

left a comment

Great job! Thx, @H--o-l 👍

@mhevery mhevery closed this in 81c2a94 Jun 7, 2019

mhevery added a commit that referenced this pull request Jun 7, 2019

fix(service-worker): avoid uncaught rejection warning when registrati…
…on fails (#30876)

Service worker API `navigator.serviceWorker.register` can fail in multiple ways.
For example, in Chrome, with an unstable network connection you can have the
following error: `An unknown error occurred when fetching the script.`

In the current state, it creates an `Uncaught (in promise) TypeError:` style of
error, which cannot be caught by the user on his own.

I think it's better to log the error over raising an error that cannot be
caught.

PR Close #30876
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.