Bugsnag-js: Settings when used as a module not honored

[bathos]

The docs state:

In situations where Bugsnag is not in its own script tag, you can set this with:

Bugsnag.autoNotify = false;

However bugsnag does all the patching which is contingent on this being true (the default) before the value can actually be set — it seems it is only actually possible to control this behavior if you inject bugsnag via a script element. Since the main reason to disable autoNotify is to prevent overzealous patching of performance sensitive functions like requestAnimationFrame, it becomes necessary to do things like:

const raf = window.requestAnimationFrame;
const bugsnag = require('bugsnag');
window.requestAnimationFrame = raf;

Rather awkward! Plus, if you aren’t concerned with stuff like IE6 support, all of the various mutations of the global space seem pointless. Why is this patching enabled by default, why isn’t it disclosed in the documentation, and why do the docs state that autoNotify can be meaningfully set this way?

---

_This is what I came up with BTW for anybody else trying to use bugsnag without the global decoration. In my case I actually do want bugsnag to register an onerror handler, but I don’t want it to do all the wrapping. This module needs to be imported prior to bugsnag and its exported function needs to be invoked afterwards:_

const restorations = [
  [ 'ApplicationCache',          'prototype', 'addEventListener' ],
  [ 'AudioTrackList',            'prototype', 'addEventListener' ],
  [ 'ChannelMergerNode',         'prototype', 'addEventListener' ],
  [ 'CryptoOperation',           'prototype', 'addEventListener' ],
  [ 'EventSource',               'prototype', 'addEventListener' ],
  [ 'EventTarget',               'prototype', 'addEventListener' ],
  [ 'FileReader',                'prototype', 'addEventListener' ],
  [ 'HTMLUnknownElement',        'prototype', 'addEventListener' ],
  [ 'IDBDatabase',               'prototype', 'addEventListener' ],
  [ 'IDBRequest',                'prototype', 'addEventListener' ],
  [ 'IDBTransaction',            'prototype', 'addEventListener' ],
  [ 'KeyOperation',              'prototype', 'addEventListener' ],
  [ 'MediaController',           'prototype', 'addEventListener' ],
  [ 'MessagePort',               'prototype', 'addEventListener' ],
  [ 'ModalWindow',               'prototype', 'addEventListener' ],
  [ 'Node',                      'prototype', 'addEventListener' ],
  [ 'Notification',              'prototype', 'addEventListener' ],
  [ 'requestAnimationFrame' ],
  [ 'Screen',                    'prototype', 'addEventListener' ],
  [ 'setImmediate' ],
  [ 'setInterval' ],
  [ 'setTimeout' ],
  [ 'SVGElementInstance',        'prototype', 'addEventListener' ],
  [ 'TextTrack',                 'prototype', 'addEventListener' ],
  [ 'TextTrackCue',              'prototype', 'addEventListener' ],
  [ 'TextTrackList',             'prototype', 'addEventListener' ],
  [ 'WebSocket',                 'prototype', 'addEventListener' ],
  [ 'WebSocketWorker',           'prototype', 'addEventListener' ],
  [ 'Window',                    'prototype', 'addEventListener' ],
  [ 'Worker',                    'prototype', 'addEventListener' ],
  [ 'XMLHttpRequest',            'prototype', 'addEventListener' ],
  [ 'XMLHttpRequestEventTarget', 'prototype', 'addEventListener' ],
  [ 'XMLHttpRequestUpload',      'prototype', 'addEventListener' ],
].map(path => {
  const values = [ window ];

  for (const key of path) {
    if (values[0].hasOwnProperty(key)) {
      values.unshift(values[0][key]);
    } else {
      return;
    }
  }

  const [ value, target ] = values;
  const key = path.pop();

  return () => target[key] = value;
}).filter(Boolean);

export default () => restorations.forEach(restore => restore());

[kattrali]

Thanks for the report, @bathos. It sounds like a rethinking of a lot of this logic (and workarounds) is needed, especially to put users in better control of which browsers they care about. I'm updating the roadmap to ensure this is addressed soon.

[bathos]

Sounds great — fwiw I think you’ve hit the nail on the head, being able to say "I want to support the following browsers" would be a reasonable level of abstraction for configuring this stuff & communicating its purpose.

Another related approach would be to retain the automatic-ness of things but make the patching conditional on the execution context. The environments that need this patching to capture errors with their stacks — which are now relatively a lot less common, thankfully — are generally detectable, I think.

[bengourley]

Hey @bathos, it's been a while since you raised this issue but I wanted to let you know we're on the verge of releasing a new major version of the notifier (v4) which now does away with all of this patching behaviour.

If you'd like to try it out in its beta phase, please see: https://docs.google.com/document/d/1HBBLyVv9FHsNmbkRLx58WbtisMRVHwDoR3pnxac9JQQ/

Otherwise if you'd like to wait until the "stable" 4.0.0 release, that should arrive at the start of Dec. If you try the beta out let us know how you get on 🙂

[bathos]

The update sounds fantastic! I happen to be working on a project that isn’t in prod yet so it’s a perfect time for me to do some test driving.

[bathos]

I dunno if this is useful, but here are some initial haven’t-actually-run-it-yet thoughts:

• Minor suggestion: I see it’s in ES2015+, but Common JS. If it used ES modules and a "module" field were in package.json, it’d be a little friendlier to tools like Rollup (and just in general, the future), and the pre-transpilation entrypoint would be clearer. That is, I will be using this by importing from browser/index.js directly, since we’re doing multiple tiers of transpilation for different targets (the majority of our users don’t need any of the transforms here, so the payload ends up smaller/faster/easier to debug).
• Thrilled to see that you _didn’t_ go isomorphic/universal. Although we use it in such a context, it’s better to load context-specific libs conditionally and configure the build accordingly so that you don’t end up sending a bunch of unused node-specific cod to the client. So this is a big :thumbsup: for me.
• Regarding unhandledrejection, it might be nice to be able to configure its severity. In patterns that _reuse_ promises as long-lived result-values — which is actually pretty powerful, though uncommon — you end up getting false positives on this event. Not gonna affect me, but it’s something I’ve seen in the past.

All around it looks great and I’m excited to play around with it (post-thanksgiving I guess ... ride’s almost here, haha).

[bengourley]

v4 is now published 🚢