Closed JamieMason closed 4 years ago
ugh IE again lol.
i'd welcome a PR! let's actually change the line var hasArrayBuffer = typeof ArrayBuffer === 'function';
and add a check for isView
. It'd be nice to comment to note the IE fix. One of the goals of this package is to be very small, so try and think thru the minimal way to safely create hasArrayBuffer
Hm that’s odd... MDN flags ArrayBuffer.isView
as supported on IE11 🙃 https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/ArrayBuffer/isView
what would be the most appropriate file to put a test in for this? test/browser/browser.spec.js
, one of test/node/*.spec.js
, or create a new file for testing host environment conditions?
@JamieMason this is still an issue with IE11 right? i'll keep it open if it is
Hi @chrisbolin, this is still present yes – have reopened.
On ArrayBuffer.isView
being supported in IE11 – another theory might be a bad Polyfill somewhere in the Stack, or one released in the period between ArrayBuffer
and ArrayBuffer.isView
being adopted – I don't know. The environment is outside of our control but we can take small measures to defend ourselves against it.
EDIT:
Totally off-topic, but I played around with some optimisations in this branch https://github.com/JamieMason/react-fast-compare/pull/new/optimisations and measured consistent improvements, it spoils the boundary you've been trying to maintain via comments between this and your upstream library though, but it's there incase you're interested in the ideas.
@JamieMason -- Is there perhaps a simple reproduction (no minification, etc.) you can host online / in a public repository that we can then open in an ie11 VM and confirm?
A reduced test case can be helpful to isolate a problem when there's a lot of noise and can be a really good idea, but could you help me understand why one would be needed in this case @ryan-roemer? Let me explain my thinking – the only variable I can think of is whether IE11 supports ArrayBuffer.isView
or not, and our VM could assert that via its Developer Console.
We can also see how this library would behave under the condition that ArrayBuffer
is present but ArrayBuffer.isView
is not by reading the source.
So I think the only question left is whether we feel that, on balance, it's worth doing. It makes an uncaught exception impossible, but in old and dying browsers, and adds code that now has to be maintained.
So, when I do just that test in IE11 vm via https://www.browserling.com/ it's all there:
So I think if we think the behavior you're seeing is a bug in react-fast-compare, I'd love to see a failing test case that uses react-fast-compare and throws on ie11...
I'm totally happy to close it Ryan
it makes sense that it's tough to repro, as the original error came from Sentry—so not even a machine you controlled
So whether it happens in specifically IE11 is a Red Herring, whether it does or doesn't we still know that Browsers exist with this level of intermediate support and, just like IE 11, they're way out on the fringes.
the only question left is whether we feel that, on balance, it's worth doing. It makes an uncaught exception impossible, but in old and dying browsers, and adds code that now has to be maintained.
It's such low priority you could live with it and be fine and I honestly don't mind either way whether you implement a fix or not, I'm not invested and have no reason to sell you it – I saw the error in Sentry and thought it helpful to bring it to your attention.
I think you have all the information you need so I'll end my involvement here. Thanks for your time and for this great Project 👏
Hey team,
This has been popping up in our error logs recently, which is how I stumbled across this issue. At least in our codebase it appears via react-helmet-async. The errors are all IE11 (except for two IE Mobile 11).
Any chance you have a simple reproduction that uses the library that errors that we can try in ie11?
This is probably quite out there, but according to this from 2016 there are older versions of IE 11 that don’t support it... https://github.com/mapbox/pbf/issues/72 I don’t really know what to say. IE 11 is one thing, but... old versions of IE 11 😆
Here you go Ryan: https://jamiemason.github.io/react-fast-compare/
Thanks @JamieMason and @antonjb! We should have a patch out soon.
I'm still not getting the reproduction on ie11, so maybe it's an old ie11 thing or whatnot as mentioned above.
At any rate, here's a suggestion for us for a guard at top:
var hasArrayBuffer = typeof ArrayBuffer === 'function' && typeof ArrayBuffer.isView === 'function';
or a slightly more terse version that just uses prop existence instead of function test:
var hasArrayBuffer = typeof ArrayBuffer === 'function' && !!ArrayBuffer.isView;
Thanks, there is a branch ready to put forward as a PR in https://github.com/FormidableLabs/react-fast-compare/issues/66#issuecomment-617298760 incase that would be of any use.
Thanks @JamieMason! -- we're super concerned with overall gzip size and the "terse version" I have above I'm pretty sure will give us smallest overall size (and avoids unneeded functions calls vs. an extra noop function call).
Thanks too for the optimizations branch, but I'm guessing they will actually get bigger for overall gzip size as gzip usually does better without aliases like var ObjectPrototype = Object.prototype;
, etc. You can check out with yarn size-min-gz
in dev vs. current master.
Thanks a lot @ryan-roemer. On the fix branch that is a good point about gzip-suitability, I hadn't factored that in.
To add some context on the optimizations branch I poked around with – the changes there were only around raw compute speed, rather than helping a minifier or gzip reduce the file size – so those changes like var ObjectPrototype = Object.prototype;
improved performance (negligibly) by caching the result of getting Object
then reading prototype
from it every time. This caching optimization is really small, but appears in a recursive code path – so I guess it could add up when comparing larger datasets.
I added some Benchmarks in a collapsed <details>
element in https://github.com/FormidableLabs/react-fast-compare/issues/66#issuecomment-617298760. The tl;dr is a small improvement, these are the numbers from your "general and react" suite.
branch | worst ops/sec | best ops/sec |
---|---|---|
master | 79,449 | 86,828 |
optimizations | 85,835 | 91,610 |
I hadn't measured the filesize but I imagine it would likely gain a little weight.
I'm still not getting the reproduction on ie11, so maybe it's an old ie11 thing or whatnot as mentioned above.
I couldn't get it either. Had a look at the user agent string of one of the issues in our error tracking. For interest: Mozilla/5.0 (Windows NT 6.1; WOW64; Trident/7.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; .NET4.0C; .NET4.0E; rv:11.0) like Gecko
Operating system: Windows 7
Tried using a simulated browser and Jamie's example...and still don't get it.
Hi @JamieMason @antonjb - we just released this fix in v3.0.2
. Can you upgrade and let us know if you're still seeing this err? Thanks again for bringing this to our attention.
Will do, thanks @kale-stew
That's awesome, thanks for addressing the issue team. React Fast Compare arrives to our codebase as a dependency so I'll need to head to those repos and see if it can be updated.
Hi, I noticed in a bug report from Sentry that /index.js#L67 throws
Object doesn't support property or method 'isView'
in IE 11.Broadly speaking, I think the fix would be to check for
ArrayBuffer.isView
before invoking it:I'm happy to submit a PR if you agree to this change.
Thanks.