ably / ably-js

Javascript, Node, Typescript, React, React Native client library SDK for Ably realtime messaging service
https://ably.com/download
Apache License 2.0
310 stars 55 forks source link

Review implications of using Uint8Array for Vcdiff plugin on browser support #659

Open QuintinWillison opened 4 years ago

QuintinWillison commented 4 years ago

Looking at our implementation at BufferUtils.toBuffer and its use of ArrayBuffer and Uint8Array, my understanding from what I'm reading is that these require a minimum of Internet Explorer 10.

However, our read me states that we support down to Internet Explorer 8.

Clearly I've picked on just one browser here but it does make me wonder whether we've fully assessed for all the popular browsers. Plus, of course, bringing a Vcdiff decoding plugin in to the mix for version 1.2 as well.

There may be a wider story here which I'm going to muse on under this issue but might consider creating another issue for at the point we start work on this focussed matter... My musings are:

  1. Should we be running something like JSHint over our browser 'built' outputs to ensure the source is compatible to the level we state in our docs?
  2. Can we find a tool which can identify which browser APIs our code relies upon or is that simply impossible given we're outputting pure JavaScript... maybe this is part of a bigger story around moving to TypeScript (or similar) tech?
SimonWoolf commented 4 years ago

FWIW: the plan was always for deltas to only work in browsers that support ArrayBuffer & Uint8Array - the vcdiff plugin we're using was written to use them; we were just going to tell people that if they need IE8/9 support they shouldn't use deltas. This was agreed with Tsviatko & Paddy, but looks like we may not have written it down anywhere that I can find, so that's a failure of communication on our part - sorry.

With IE8 and 9, the e2e encryption feature works using CryptoJS wordArrays. The BufferUtils functions that are needed by the encryption code do not assume the presence of ArrayBuffer etc., they work with wordArrays too.

BufferUtils.toBuffer does assume ArrayBuffer; that's because that function is only used (other than deltas) in msgpack encoding. On IE8 and 9, Platform.supportsBinary is false, so the library can't and doesn't use msgpack anyway (it uses JSON over the wire instead).

The closure compiler effectively acts as a sort of linter to enforce ES3 syntax, which helps. I definitely do agree we need better linting -- https://github.com/ably/ably-js/issues/441 . Unfortunately I don't think a linter (or any static analysis tool) can easily guarantee that (say) IE8 will never encounter a codepath that uses ArrayBuffer at runtime -- the logic preventing it is too dynamic. (Well, it could of course just ban all usage of ArrayBuffer, but that would prevent us having features like msgpack and delta support on browsers that are new enough to support them, which we don't want to do).

So historically we've relied on the browserstack tests to make sure we don't get a regression on IE8 for the set of features we do support on it. (Which admittedly have their own problems).

SimonWoolf commented 4 years ago

(relatedly: now that Microsoft has (finally) ended support for IE8, we should consider dropping support for it, after checking usage is sufficiently low. Then we could at least assume ES5 support)

QuintinWillison commented 4 years ago

Thanks. An ES5 baseline would surely make life easier. Your comments here will be really useful when we come to work on this.

paddybyers commented 4 years ago

now that Microsoft has (finally) ended support for IE8, we should consider dropping support for it, after checking usage is sufficiently low

My memory of it is that IE8 is the newest browser that works on XP, and therefore it's going to take a long time to die. So really the question will be whether or not we will drop support for XP.

paddybyers commented 4 years ago

Referring to https://en.wikipedia.org/wiki/Internet_Explorer_8#Adoption, I see:

image

On that basis, I think I'd support dropping support for IE < 11. We should check our stats/logs if we can.

SimonWoolf commented 4 years ago

On that basis, I think I'd support dropping support for IE < 11

that would be nice -- even getting rid of 9 would let us remove the jsonp transport and all the cryptojs wordarray fallbacks

mattheworiordan commented 4 years ago

@SimonWoolf @paddybyers what's stopping us from perhaps introducing LTS for 1.1, where we will fix bugs if they arise, but ultimately from 1.2 onwards we only support reasonable browsers i.e. ones that are not older than 10 years!

With that approach, we could get rid of the cruft as @SimonWoolf has suggested, and we can simply tell customers who need older browser support to use 1.1.

paddybyers commented 4 years ago

even getting rid of 9 would let us remove the jsonp transport and all the cryptojs wordarray fallbacks

I am a bit uneasy about removing jsonp - it just works everywhere. But maybe the days really are gone where you can't assume XHR+CORS.

what's stopping us from perhaps introducing LTS for 1.1, where we will fix bugs if they arise, but ultimately from 1.2 onwards we only support reasonable browsers i.e. ones that are not older than 10 years!

I'd support that.