Skip to content
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

Remove some polyfills #101

Merged
merged 6 commits into from
Aug 10, 2022

Conversation

AaronDewes
Copy link

@AaronDewes AaronDewes commented Aug 9, 2022

This should reduce the runtime size & load times somewhat by removing polyfills for older browsers which are not used much.
These browsers include the non-chromium Edge and some older versions of Chrome & Firefox.

In addition, it also updates eslint

This should reduce the runtime size & load times somewhat by removing polyfills for older browsers which are not used much
Also replace eslint-plugin-node with the maintained eslint-plugin-n
Not required anymore since node-redis 3.0
@timvisee
Copy link
Owner

Very nice work once again! I glanced through it, and it LGTM.

I'll try to go through this in full and merge this later today.

@timvisee timvisee self-requested a review August 10, 2022 11:50
@timvisee timvisee self-assigned this Aug 10, 2022
@timvisee timvisee added the enhancement New feature or request label Aug 10, 2022
@timvisee timvisee merged commit 643287e into timvisee:master Aug 10, 2022
@timvisee
Copy link
Owner

This appears to break the crypto check.

This is in Firefox on macOS:

image

image

@timvisee
Copy link
Owner

timvisee commented Aug 10, 2022

crypto.subtle appears to be missing. I guess a polyfill is required for that.

I accidentally merged in error. I may revert this on master if we can't quickly solve this.

@AaronDewes
Copy link
Author

crypto.subtle is only available on HTTPS sites and localhost, which is what is causing this. But I'll re-add the polyfill if you develop on other HTTP domains

@timvisee
Copy link
Owner

crypto.subtle is only available on HTTPS sites and localhost, which is what is causing this.

That makes a lot of sense.

@timvisee
Copy link
Owner

timvisee commented Sep 4, 2022

I have reverted these changes for the time being (in c624766...4ceac20) because it is causing issues for users.

Thanks a lot for your work on this though!

I'd be very happy to put this back in once we get #102 resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants