Remove `on_xhr_error` from type system

[joernroeder]

trafficstars

The configuration key on_xhr_error is marked as deprecated but looking at the code on closer inspection, calls to it have been removed and the callback is no longer being called, breaking functionality.

[joernroeder]

the code was removed here https://github.com/PostHog/posthog-js/pull/898

[joernroeder]

👋

[pauldambra]

hey @joernroeder we tend to avoid breaking changes as much as possible... since folk loading posthog with the snippet will get the latest version if they are providing on_xhr_error then their tracking would stop working if we remove it...

But, it should be that we're safely ignoring it... is it breaking things for you? Or only confusing that it's present?

[joernroeder]

You guys broke the functionality by keeping the types but removing the implementation which left us (and others potentially) in the dark. The callback didn't get called anymore when it should have. For us that was bad, last time you guys had an outage our "on error" logic didn't trigger, causing ripple effects which we prevented by relying on the on error callback in the first place.

Worst part, no way for us to realize that the handler got removed. Semver, did not indicate breaking changes, not the types. deprecating it, adding console warning, while keeping the functionality alive, plus appropriate semver bump would have been the right move imo.

We now have playwright tests which test for outages and correct behavior but sadly had to learn the hard way.

Edit: we don't load the snipped but use the client provided by the npm package.

[pauldambra]

Ah, fair... that's a pain and we should have realised and avoided it... i can only say sorry 💖

[pauldambra]

wait... i can do more than apologise...

we can at least warn the future traveller https://github.com/PostHog/posthog-js/pull/1298

(sorry again, we try to be super careful with the SDKs but we didn't manage it here 😞 )

(we have to keep it in the type system since it'll break folk in a different way to remove it, so I'll close this but hopefully not taken as dismissive of the impact this had on you)

[joernroeder]

you could add a @deprecated jsdoc comment on the typescript prop, many editors pick that up and show it struck through which would give devs a hint.

[pauldambra]

Ah... we've hit similar here at PostHog too with the difference between // comments and /** */ comments between pycharm and vscode...

I use jetbrains IDEs which tend to be much better at picking things up so on_xhr_error does show as struck through for me... 🤔

[pauldambra]

i happen to have something open in vscode to check and see this...

is that what you mean?

[joernroeder]

After sleeping on this I think the best move would have been to do something like this dummy code below:

1. deprecation notice for folks who are going to use it in their editor
2. console.warn for logging monitoring to pick it up
3. maintain functionality by forwarding to the new function

{
  /**
   * @deprecated use `on_request_error` instead.
   */
  on_xhr_error: (args) => {
    console.warn('on_xhr_error is deprecated, use on_request_error instead')
    return this.on_request_error(args) // use .bind, .call etc here but you get the idea
  }
}

[pauldambra]

Totally agree :) i think it'd be closing the stable door after the horse has bolted now but this absolutely 100% should not have been breaking for anyone - like i say we try to be super careful on this and we didn't manage it with this change