Angular change detection problems in Replay

[billyvg]

Angular monkeypatches browser APIs so that when they are called, will trigger Angular's change detection. This means that our SDK can end up causing customer applications to unnecessarily re-render, which in turn causes our Replay SDK to perform more work and can even cause performance regressions.

Some example areas where this happens:

• In the rrweb-snapshot package, snapshot.ts is using the global setTimeout and clearTimeout functions, which are monkeypatched by Angular and triggers their change detection.
• Promise
• Any EventTarget (e.g. window, Performance, Worker)

Related to #11661

[billyvg]

This should help on the rrweb side of things: https://github.com/rrweb-io/rrweb/pull/1509/files

[billyvg]

I think there are a few potential solutions:

Continue using getNativeImplementation()

This has slight overhead for all browser packages as we need to check if the fn we're using is a native fn. There is also a slight hit as we have to use DOM APIs to create an iframe to get the native implementation (this happens per first function call, so we could try to pre-load this in the Angular SDK as an optimization).

Run outside of Angular Zone

I'm not super familiar with Angular, but I think this could work if new async calls made outside of the zone continue to be outside. Then we could have the Angular SDK wrap browser/init() with runOutsideAngular(). However, this wouldn't cover the case where users call Sentry APIs directly, as well as other edge cases. It will be difficult to target specific methods to override from the Angular SDK.

Set APIs at run-time

Another option would be having the Angular SDK set the APIs at run-time before we initialize replayIntegration. This could mean we could avoid calling getNativeImplementation for all non-angular SDKs at the expense of always calling a getter (as well as some developer ergonomics). This would look something like the following:

1. a setter function to set a map of the clean APIs. This would be stored at the module level. By default this will use the current global APIs, but Angular would call it with the cleaned versions. e.g.

setApis({
  setTimeout: getNativeImplementation('setTimeout'),
  ...
});

2. an exported map of getters. Otherwise, the imports would reference the value of the export at the time of import, rather than when the exported property is accessed.

export const apis = {
  get setTimeout() { return _foo.setTimeout; }
  ...
};

...

import {apis} from '@sentry-internal/browser-utils';

apis.setTimeout(() => ...);

All three solutions are easy to regress as it's normal for a developer to call the globals directly, but this can likely be mitigated by a linter.

cc @Lms24 when you're back as you're probably the only one with any Angular experience :P

[Lms24]

I will bring this up with the team next week if they have preferences but I agree, none of these options seem great.

my thoughts (please correct me if I'm wrong)

• If we can rely on the linked PR for rrweb, we can at least eliminate one class of zone problems, namely the API calls within rrweb, right? Should we wait for this and check the impact? IIRC we already do this in our rrweb fork but maybe I'm off here
• As for generally accessing window et.al., maybe we should go down the apis map route. Zone is not the only library that patches browser APIs, so this might be beneficial in general 🤔 Though it has a bundle size hit (like all proposed solutions)
• Running Sentry.init outside of the angular zone might not do as much as we hope (admittedly untested). But given that our SDK should be initialized before Angular bootstraps itself might imply that Zone only inits and patches after Sentry.init, meaning it doesn't make a difference 🤔
• Can't wait for Angular to become zone-less going forward. Though we only benefit from this once people actually migrate to that new Angular version (whenever this step is made). So realistically, this will haunt us for years.