fix(service-worker): handle error with console.error

[H--o-l]

This commit reverts commit fix(service-worker): handle error with
ErrorHandler.

With Angular v11.0.4 and commit fix(service-worker): handle error with
ErrorHandler
Angular start to send all service worker registration errors to the Angular
standard ErrorHandler#handleError() interface, instead of logging them in the
console.
But users existing ErrorHandler#handleError() implementations are not adapted
to service worker registration errors and it might result in broken apps or
bad UI.
Passing to ErrorHandler is desirable for some and undesirable for others and
the same is true for passing to console.error().
But console.error() was used for a long time and thus it is preferable to keep
it as long as a good solution is not found with ErrorHandler.

Right now it's hard to define a good solution for ErrorHandler because:

1. Given the nature of the SW registration errors (usually outside the control
   of the developer, different error messages on each browser/version, often
   quite generic error messages, etc.), passing them to the ErrorHandler is
   not particularly helpful.
2. While ErrorHandler#handleError() accepts an argument of type any (so
   theoretically we could pass any object without changing the public API), most
   apps expect an Error instance, so many apps could break if we changed the
   shape.
3. Ideally, the Angular community want to re-think the ErrorHandler API
   and add support for being able to pass additional metadata for each error
   (such as the source of the error or some identifier, etc.). This change,
   however, could potentially affect many apps out there, so the community must
   put some thought into it and design it in a way that accounts for the needs
   of all packages (not just the SW).
4. Given that we want to more holistically revisit the ErrorHandler API, any
   changes we make in the short term to address the issue just for the SW will
   make it more difficult/breaky for people to move to a new API in the future.

To see the whole explanation see GitHub PR #40236.

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features) => too specific, no doc

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?

When implementing ErrorHandler.handleError() interface, we don't know if the error is from service worker registration or something else, and that can result in bad UX.

What is the new behavior?

Service worker registration errors are send to console.error() not ErrorHandler.handleError()

Does this PR introduce a breaking change?

• Yes
• No

[alfaproject]

Shouldn't the source be more like a code? Something like: SW_REGISTRATION

[H--o-l]

Nice idea.
But I'm not sure it would be better.
I don't know how other Angular users use ErrorHandler.handleError() but for me, it has two purposes:

1. log the error inside a tool like Sentry.io
2. warn the user of the application if needed.

SW_REGISTRATION would be better for the 2. use case, but for the 1. it will make things harder to read and parse for a human being.
Typically on Sentry, for a human, it's easier to have:

{srouce: "Service worker registration", error: "DOMException(SecurityError: The operation is insecure.)"}

Than

{srouce: "SW_REGISTRATION", error: "DOMException(SecurityError: The operation is insecure.)"}

But I have no strong opinion and I'm happy to follow your lead if you think that SW_REGISTRATION is better.

[adrienverge]

I agree that a code (like SW_REGISTRATION) would be easier to process.

[gkalpak]

I am not sure I like this change 😅

I want to understand your use case better. More specifically, I am trying to understand these statements:

But service worker registration errors are a lot different than other code errors:

• They are frequent, for example, they happen a lot with Chrome v84, Chrome v85, Firefox 84, and Samsung Internet v13.

Why do ServiceWorker registration errors happen on these brwosers? I mostly use Chrome and I haven't seen any errors in recent versions.

• Most applications can run even without a registered service worker.

This is not exclusive to ServiceWorker registration errors. This can be true for other errors as well.

[H--o-l]

Thanks @gkalpak for your feedback!

Why do ServiceWorker registration errors happen on these brwosers? I mostly use Chrome and I haven't seen any errors in recent version.

Indeed sorry I didn't give context about this statement of mine.

Inside my modest application, I log every console.error and ErrorHandler.handleError() to a logger like Sentry.io

I have a few Service worker registration errors that are logged:

• Error(Error: Rejected) on Chrome 84.0.4147 and 85.0.4183, for Windows 7 and Windows 10 plus Samsung Internet 12.1, and Edge 18.18363. It's the error that happens the most, I try to reproduce it on a windows computer and CHrome 84 without success. I don't see it on Chrome 86 and 87.
• DOMException(SecurityError: The operation is insecure.) on Firefox 84.0 mostly, but I think it's on all Firefox versions. It could be users with incognito mode. I didn't try to reproduce it.
• Error(Error: Uncaught (in promise): {"name":"not_found","message":"missing"}) on Samsung Internet 13.0
• Error(Error: Uncaught (in promise): {"name":"not_found","message":"missing","reason":"missing"}) on Samsung Internet 13.0 too.
• DOMException(AbortError: Failed to register a ServiceWorker: ServiceWorker script evaluation failed) on really old Chrome 47.0.2526 and 46.0.2490 and 48.0.2564

And maybe a few others that I didn't notice.

These errors are really hard to debug for me because I see them only on production, and I have no line numbers or anything to work with, so I can't open an issue or a PR for them.

My point in this PR is just that when ErrorHandler.handleError() is called with that kind of errors, it is valuable for me (and maybe for others?) to know that they come from a failed navigator.serviceWorker.register() so I can keep track of them, ignore them, or anything else.
Due to the variety of errors, I think I have no better way to do that than this PR, or something similar.

This is not exclusive to ServiceWorker registration errors. This can be true for other errors as well.

I didn't think of that, and you are right.
But in practice, in production, only Service worker registration errors are frequent enough that I want to do a special case for them.

Maybe you see a better way, more accurate, to formulate the commit message?

[adrienverge]

Hello,

Since we updated our app to Angular 11.0.4, our users had 4000 errors in a week because of this problem! Thanks @H--o-l for providing a solution 👍

Why do ServiceWorker registration errors happen on these brwosers? I mostly use Chrome and I haven't seen any errors in recent version.

I can't explain why some browser versions are more impacted than others (it's strange that Chrome 84 and 85 have higher rates), but it does happen a lot in production.

@gkalpak if you don't like this change, what fix would you suggest? My problem is that Angular 11.0.4 introduced a change that directly impacts users, because service worker registration errors are now considered as real runtime errors (although they're not a problem), and handled as such (reload the app). One would need a mean to differentiate them.

[adrienverge]

I agree that a code (like SW_REGISTRATION) would be easier to process.

[gkalpak]

Thx everyone for chiming in. Based on what I've read, I realize that the SW registration can fail due to factors that are not controlled by the app itself (including the framework and 3rd-party libs). This, I believe, it a differentiating factor that warranties not passing the error to the ErrorHandler at all.

At the same time, I think it is valueable to allow people to capture SW registration errors if they want to.

So, how about this:

We enhance the SwRegistrationOptions to support passing a handleRegistrationError function that would be used to log registration errors. It would default to err => console.error(err) (giving the same behavior as before #39990), but it would be possible for people to override.

A couple of examples:

1. Send registration errors to console.error() (default behavior):

   @NgModule({
     ...
     providers: [
       ...
       ServiceWorkerModule.register('...'),
     ],
   })

2. Send registration errors to external app monitoring tool:

   @NgModule({
     ...
     providers: [
       ...
       ServiceWorkerModule.register('...', {
         handleRegistrationError(err) {
           sendToExternalAppMonitoringTool({source: 'SW_REGISTRATION', error: err});
         },
       }),
     ],
   })

3. Send registration errors to ErrorHandler:

   import {..., ErrorHandler} from '@angular/core';
   import {..., SwRegistrationOptions} from '@angular/service-worker';
   
   @NgModule({
     ...
     providers: [
       ...
       ServiceWorkerModule.register('...'),
       {
         provide: SwRegistrationOptions,
         deps: [ErrorHandler],
         useFactory: errorHandler => ({
           handleRegistrationError: err => errorHandler.handleError(err),
         }),
       },
     ],
   })

WDYT?

[gkalpak]

/cc @chrisguttandin

[chrisguttandin]

I would also favor using a code property as mentioned by @alfaproject. Node.js, for example, does that as well. https://nodejs.org/dist/latest-v15.x/docs/api/errors.html#errors_node_js_error_codes

It would allow Angular to throw more specific errors in the future (when errors might not come from Zone.js anymore).

[H--o-l]

@gkalpak your proposition would work for me 👌

I think, by default, the behavior should be something like console.error('SW_REGISTRATION', err) and not console.error(err).
These errors happen 99,9% of the time in production, not during development, thus you do not have a clear stack-trace to work with. If the info that it's from service worker registration is not here right out of the box, these errors will probably go unnoticed for most users.

[chrisguttandin]

This is of course only my personal opinion, but I think the default behavior should never be to ignore an error. I think it should always be an explicit decision to ignore an error.

Logging an error to the console that most likely only happens in production in older browsers is very close to ignoring it in my opinion.

[alfaproject]

@gkalpak the custom error handler is always the more flexible option so I'm cool with that. Knowing the source is important as well if you make it agnostic (:

[gkalpak]

I've discussed this to the team and here are our thoughts and conclusions:

1. Given the nature of the SW registration errors (usually outside the control of the developer, different error messages on each browser/version, often quite generic error messages, etc.), passing them to the ErrorHandler is not particularly helpful .

2. While ErrorHandler#handleError() accepts an argument of type any (so theoretically we could pass any object without changing the public API), most apps expect an Error instance, so many apps could break if we changed the shape (for example to send something like {source: 'SW_REGISTRATION', error: ...}).

3. Ideally, we would want to re-think the ErrorHandler API and add support for being able to pass additional metadata for each error (such as the source of the error or some identifier, etc.). This change, however, could potentially affect many apps out there, so we must put some thought into it and design it in a way that accounts for the needs of all our packages (not just the SW).

4. Given that we want to more holisticly revisit the ErrorHandler API, any changes we make in the short term to address the issue just for the SW (such as what I proposed in fix(service-worker): handle error with console.error #40236 (comment)) will make it more difficult/breaky for people to move to a new API in the future.

Based on the above, we think that the best course of action is:

1. Revert the change fix(service-worker): handle error with ErrorHandler #39990 _{(sorry, @chrisguttandin 😅)}, so that SW registration errors are not passed to the ErrorHandler (since this was changed recently and may negatively affect existing apps).

2. Add a project to our backlog to revisit the ErrorHandler API and adjust it in order to (a) be more strictly typed and (b) allow easier programmatic inspection/categorization of the errors

For the time being, if anyone really needs to capture SW registration errors, there are work-arounds they could employ:

• Manually register the SW (instead of using ServiceWorkerModule.register()).
• Monkey-patch navigator.serviceWorker.register() to call the orginal method and handle errors.

I understand this is not an ideal situation, but at least by reverting the change we are not in a worse situation than we were before and we avoid adding roadblocks to future improvements.

@H--o-l, would you be up to updating the PR to essentially revert #39990? (If not, I would be happy to do it.)
I think it is also fine to add some context to the console.error() call as you proposed in #40236 (comment).

[chrisguttandin]

Hi @gkalpak, thanks for the detailed explanation. It all makes sense to me. The only part I don't quite get is the argument for not passing the registration errors to the ErrorHandler anymore. My experience with tracking errors in production is that the vast majority of errors are outside of the scope that I'm responsible for. Many of the errors that I see are caused by rare browser configurations and outdated extensions. However I really value to see those errors at least once and then decide to ignore them (with the help of a tool for tracking errors) instead of not knowing about the errors at all.

What if someone (that person was me) has a buggy deployment script and the Service Worker gets not deployed correctly and thus the registration always fails. Wouldn't that be awesome to see that error in a bug tracking tool?

As always, this is just my personal opinion.

[gkalpak]

I understand were you're coming from, @chrisguttandin. And I'm with you on capturing all errors by default.

The truth is, however, that this change can break people's apps (or result in bad UX). Passing to ErrorHandler is desirable for some and undesirable for others and the same is true for passing to console.error(). The decision to log to the console is just based on the fact that this was what was happening before for a long time.

Hopefully, we'll have the means to cater for all usecases in the future.

BTW, regarding your comment on capturing legit SW registration errors, here are my thoughts:

Basically, I think there are two types of SW registrations errors:

1. Random errors that are caused by the user's setup (rare browser config, bad extension, etc.). These is little the developer can do about these, so capturing them adds little value. (I am not saying capturing them is never valueable, but more often than not it isn't 😁)

2. Legit registration errors caused by a bad deployment or configuration. These errors are consistent (i.e. they are reproduced reliably) and thus could be (relatively) easily caught by an end-to-end test (either in a dev environment or against a staging or production instance).

Again, I am not saying this is a ideal situation (it's more like a "between a rock and a hard place" type of situation). Therefore, I think erring on the side of "least disruption" is reasonable 😉

[gkalpak]

In any case, thank you all for your input. It is really valuable for us to hear about different usecases and points of view.

[chrisguttandin]

Yeah, sorry for being so insistent, @gkalpak. :-) I do understand that sending the registration error to handleError() feels like a breaking change for some users of Angular.

What I said is just my opinion and I totally get that I'm not the only user of Angular. :-)

[H--o-l]

Thanks for your work @gkalpak and @chrisguttandin.

@H--o-l, would you be up to updating the PR to essentially revert #39990? (If not, I would be happy to do it.)
I think it is also fine to add some context to the console.error() call as you proposed in #40236 (comment).

I updated the PR and the commit message. For the commit message, I tried to be as clear as possible and re-use the explanations you give us.
Don't hesitate to amend the commit message or the commit code if needed.

[gkalpak]

Thx for making the changes, @H--o-l 👍
I have one question. Otherwise lgtm 🚀

[gkalpak]

In #40236 (comment), @H--o-l suggested using console.error('SW_REGISTRATION', err) instead of console.error(err) to give context on where the error (which often has a generic message) comes from.

In #40236 (comment), I said it sounded like a good idea and prompted @H--o-l to go ahead and make this change.

Looking at this, I thought it would be a good idea to use a more human-readable message (since this error is logged to the console and is not processed programmatically (i.e. it is aimed for being read by humans).

(Interestingly, I then realized that that is exactly what we were doing before #39990: console.error('Service worker registration failed with:', err))

So, my question is: Am I missing something? Is there any benefit in using SW_REGISTRATION instead of something more descriptive such as Service worker registration failed with:?

[H--o-l]

One benefit I see: if in the future Angular use more error code, like SW_REGISTRATION, inside the SW module we will already be used to SW_REGISTRATION.

since this error is logged to the console and is not processed programmatically

Not sure about this, you could try to mock console.erro() like the following and then processed it programmatically:

console.error = patchedConsoleError;

(Personally, I don't have a strong opinion on the subject)

[gkalpak]

since this error is logged to the console and is not processed programmatically

Not sure about this, you could try to mock console.error() [...] and then process it programmatically

Sure you can monkey-patch console.error() like that, but then you can't expect the input to be "programmatically-processing-friendly" 😁
(And the same is true for all other errors that end up in console.error().)

So, what I meant was more that "people should not expect arguments passed to console.error() to be optimized for programmatic processing" 😃

Also, since we don't yet know what exactly error reporting will look like in the future (and whether SW_REGISTRATION will fit into that), I think it is prefarable to revert back to what we had before #39990:

Suggested change

	.catch(err => console.error('SW_REGISTRATION', err))));
	.catch(err => console.error('Service worker registration failed with:', err))));

[H--o-l]

OK, noted, I updated to PR.
Now it reverts #39990 and does only that.
It can be checked with git diff 97310d34f5 -- packages/service-worker/src/module.ts packages/service-worker/test/module_spec.ts

[gkalpak]

Thx, @H--o-l! Great commit message btw 💯
Just one typo (copied directly from my comment 😇):

-to move a new API
+to move to a new API

[H--o-l]

Thx, @H--o-l! Great commit message btw
Just one typo (copied directly from my comment ):

-to move a new API
+to move to a new API

Thanks! Fixed.

[angular-automatic-lock-bot]

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.}