stayallive / wp-sentry

A (unofficial) WordPress plugin reporting PHP and JavaScript errors to Sentry.
https://wordpress.org/plugins/wp-sentry-integration/
MIT License
305 stars 48 forks source link

Remove IE polyfill #136

Closed kkmuffme closed 2 years ago

kkmuffme commented 2 years ago

WP has dropped IE11 support with WP 5.8 a year ago. I think the polyfill in wp-sentry-browser .js can be removed too?

Perhaps not run sentry at all if if (!String.prototype.startsWith) { ?

stayallive commented 2 years ago

From the looks of it I should actually include more polyfills considering the Sentry SDK still supports IE 10 & 11.

https://docs.sentry.io/platforms/javascript/troubleshooting/supported-browsers/

This plugin also works on older WordPress versions not just the newest (technically we support version 4.4 and up) so I don't see much reason to drop the very light polyfill we have at this moment.

I'm considering adding the documented polyfills required for the SDK to function properly. Possibly I'll make this optional so we can move the current polyfill to there too which might solve both problems?

kkmuffme commented 2 years ago

I think the doc you are referencing is outdated (as is the case incredibly often with Sentry unfortunately):

The v7 SDK is ES6 and does not support IE11 by default: https://github.com/getsentry/sentry-javascript/blob/7.0.0/MIGRATION.md#upgrading-from-6x-to-7x and https://github.com/getsentry/sentry-javascript/blob/7.0.0/MIGRATION.md#moving-to-es6-for-commonjs-files

stayallive commented 2 years ago

I know this 😉

https://github.com/stayallive/wp-sentry/blob/e697fe5b28ddc014c358e9c0738d95e597e274af/bin/update-js-files.php#L13

So the ES5 bundle is the one we bundle already, we should probably stop doing that and make ES5 including the polyfills optional for IE 10+ support. I'm assuming most don't care anymore these days.

kkmuffme commented 2 years ago

Since the official support for these versions is dropped by Microsoft itself, I think there is no reason to care about those anymore.

stayallive commented 2 years ago

This is not how Sentry operates, the SDK philosophy is to support as wide as possible.

https://develop.sentry.dev/sdk/philosophy/#compatibility-is-king

So as long as we can easily allow IE 10+ support by bundling the ES5 version + a little polyfill I see no reason to remove support completely.

I agree that it's probably of little use to anyone but since the cost is so incredibly low I'd be happy to keep it for now 😄

Again, with the plan I have you will not get the polyfill or ES5 bundle anymore in the next version.

kkmuffme commented 2 years ago

That's nice for Sentry, but this plugin is sentry for Wordpress :-) And Wordpress does not support those old versions. Why would I want to log errors from versions I know, are not compatible with my code/WordPress?

stayallive commented 2 years ago

As I mentioned in my very first comment, we support WordPress version 4.4 and up.

https://make.wordpress.org/core/handbook/best-practices/browser-support/

Version 4.8 dropped support for IE 10 and 5.8 dropped support for IE 11.

So it's very much relevant to this plugin, since the lowest supported WordPress version (4.4) supports IE 10 & IE 11.

I don't see the issue though, I mentioned several times that in a future update I'll make the ES5 bundle (and thus IE 11 support) opt-in (disabled by default), so there is no downside for you as far as I can tell of this plugin supporting it.

stayallive commented 2 years ago

Fixed in: https://github.com/stayallive/wp-sentry/releases/tag/v6.0.0