-
Notifications
You must be signed in to change notification settings - Fork 55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ECO-4204] Remove code that's supporting older platforms (in v2) #1629
Conversation
6af3284
to
3d41166
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There still seem to be some mentions of IE:
ably-js/test/browser/connection.test.js
Line 21 in 3d41166
// IE doesn't support creating your own events with new var xdr = new XDomainRequest(); /* Use IE-specific "CORS" code with XDR */
and of browser compatibility accommodations:
Line 22 in 3d41166
// comma-dangle used for browser compatibility for browsers that don't support trailing commas
Also, just a heads-up that writing "Resolves #1538 , #1332" in your PR description doesn't do what you think it does — you need to write "Resolves #1538, resolves #1332".
Initially I wasn't sure if I wanted to remove IE code from tests that would lead to those tests not even being able to run on IE, but now looking at it again it really doesn't make sense to have this code present anymore. We're never going to test against IE and if someone else would try to run those tests - well it is going to break for a reason - we don't support IE anymore. So I will remove those mentions from test cases too.
Will take a look.
Right, thanks! |
This is in preparation for unifying IPlatformConfig.getRandomValues and IConfig.getRandomArrayBuffer functions into one. `getRandomArrayBuffer` implementation details for each platform: - for `nativescript` - based on nativescript's `getRandomValues` function - for `nodejs` - based on `generateRandom` function from our nodejs crypto code [1] - for `react-native` - was already implemented - for `web` - based on `generateRandom` function from our web crypto code [2]. After fccd4ac it is possible to simplify this code by removing `DEFAULT_BLOCKLENGTH_WORDS` and using Uint8Array directly instead of Uint32Array and calculating words [1] https://github.com/ably/ably-js/blob/d0e9fbe4797ebb8520286592ba7b5d82a139eb39/src/platform/nodejs/lib/util/crypto.ts#L32 [2] https://github.com/ably/ably-js/blob/d0e9fbe4797ebb8520286592ba7b5d82a139eb39/src/platform/web/lib/util/crypto.ts#L35
In #1537 we established browser versions we intend to support in ably-js v2 going forward, and all of them support `Crypto.getRandomValues`, thus there is no need to have fallbacks to `Math.random` anymore. This commit removes such code and changes corresponding places to always call IPlatformConfig.getRandomArrayBuffer instead. Resolves #1332
e356d07
to
9a05dc8
Compare
Applied changes from conversations, removed IE mentions from tests and removed browser compatibility eslint rule |
Added 2fc88a3 to remove node 0.8.x specific code, please review |
Resolves #1538 , resolves #1332
See commit messages for more details.