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.

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?

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?

/cc @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).

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

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.

@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 (:

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

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.

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 😉

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

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. :-)

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.

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.

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.