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

v2: Drop IE8-IE10 support. #345

Open
wants to merge 22 commits into
base: v2
Choose a base branch
from
Open

v2: Drop IE8-IE10 support. #345

wants to merge 22 commits into from

Conversation

icio
Copy link
Contributor

@icio icio commented Jun 6, 2023

Analysis shows these are, thankfully, unused.

Checklist

Before merging:

  • Retarget this branch to a new v2 branch.
  • Ensure desired v1 PRs are merged
  • Rebase the v2 branch on v1.

After merging:

  • Change the repo's default branch to v2.
  • Change RavelinJS integration test in core to target the v2 branch.

timphillips
timphillips previously approved these changes Jun 6, 2023
Copy link
Contributor

@timphillips timphillips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent. I did a cursory review and the changes all look reasonable.

package.json Show resolved Hide resolved
README.md Show resolved Hide resolved
@icio icio requested a review from timphillips June 7, 2023 10:48
@icio icio changed the base branch from v1 to v2 June 7, 2023 11:00
@icio icio mentioned this pull request Jun 7, 2023
@icio icio changed the title Drop IE8-IE10 support. v2: Drop IE8-IE10 support. Jun 7, 2023
icio added 19 commits June 8, 2023 11:00
IE8-10 didn't support window.crypto.getRandomValues and our library fell back
to collecting entropy from mouse/keyboard events. There was a risk it hadn't
collected enough entropy, so we'd jiggle the mouse until it worked. IE11
supports getRandomValues so we shouldn't run into this error any more.
We've seen failures in Chrome 109 on Mojave where the Core#send beforeEach hook
has errored with:

    global leak detected: ret_nodes

    https://automate.browserstack.com/dashboard/v2/builds/d02d09eba240bfe63d5d4a38186f8b263e51448b/sessions/cf7edd2c7efa5ea1d0b8d2267290fa7bdcb0595a?auth_token=e9574e9467e1f22131acef157981aeb0791f3016cc7568e5faf4fbbf550f6402&build_token=aWYzOUl2OVQxRVBrWURhSUdKZWliZU1TV01NTStuQlNUejdRSTE1djgrWXNXWUVRZmh0d25KM3lSSHVKaC91NmFRRjRreFN6RnhMWFNsOS9pb25IM0E9PS0tejEwVU1hSlNsSHJKczZDK0Y3YjlHdz09--d2cf7eb2a908ea8939f86035ad68730cc7ea2969

I've tried to track the source of ret_nodes, but been unable to find it in our
code, our vendored code, or the test-framework dependencies in
test/unit/index.html. I think we're fine without the check.
expect is the latest version (last updated 9 years ago) and xhook 1.4 is the
last version to support IE11.
timphillips
timphillips previously approved these changes Jun 8, 2023
Copy link
Contributor

@timphillips timphillips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good apart from a few minor things.

CONTRIBUTING.md Outdated Show resolved Hide resolved
@@ -1,6 +1,6 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should update "branch": "v1", in the np section too.

</head>
<body>
<div id="mocha"></div>

<script src="https://unpkg.com/[email protected]/dist/xhook.js"></script>
<script src="https://unpkg.com/jquery@1.12.4/dist/jquery.js"></script>
<script src="https://unpkg.com/jquery@2.2.4/dist/jquery.js"></script>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The jQuery version in CONTRIBUTING.md can be bumped to v2.

Co-authored-by: Tim Phillips <[email protected]>
@icio icio requested a review from timphillips June 8, 2023 14:13
@icio
Copy link
Contributor Author

icio commented Jun 8, 2023

@timphillips You might need to do the honours from here ✌️

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

Successfully merging this pull request may close these issues.

2 participants