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

chore: update dependencies #3539

Merged
merged 39 commits into from
Jul 25, 2022
Merged

chore: update dependencies #3539

merged 39 commits into from
Jul 25, 2022

Conversation

dbowling
Copy link
Contributor

@dbowling dbowling commented Jul 11, 2022

Updates project dependencies to current versions when possible.

The following dependencies are one or more major versions behind and have not been upgraded to latest, but should be safe for minor and patch updates.

Seleniumn is pinned due to a bug, but should be easily updated in the future.

Also, it appears the selenium webdriver browser message "Chrome is being controlled by automated software" changes the window height when it appears between running the two axe runs. To fix deepEqual, we assign the height to be identical: axeRunPartialResult.testEnvironment = axeRunResult.testEnvironment;

ESBuild is upgraded to 0.14.7 (0.14.50 is latest) due to flaky windows tests. This is the version that passed CI and was reasonably up-to-date.

replaces: PR #3535

@dbowling dbowling marked this pull request as ready for review July 11, 2022 22:03
@dbowling dbowling requested a review from a team as a code owner July 11, 2022 22:03
@dbowling dbowling requested a review from michael-siek July 11, 2022 22:04
dbowling added 4 commits July 12, 2022 08:32
requested by Michael
Flakey test locally; checking CI
Requires “internet explorer”
deepEqual() assert failed because selenium now places a banner that throws off window height
@dbowling dbowling requested a review from straker July 13, 2022 18:57
dbowling added 6 commits July 13, 2022 13:10
deepEqual() assert failed because selenium now places a banner that throws off window height
v11 introduces ESM
confirmed v10 sinon does not support IE11 for CI
@dbowling dbowling self-assigned this Jul 13, 2022
Copy link
Contributor

@straker straker left a comment

Choose a reason for hiding this comment

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

LGTM. @WilcoFiers should also approve since I helped a bit.

Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

Couple minor points

Comment on lines +11 to +12
| aria-attribute | axe-core support |
| -------------- | ---------------- |
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems odd. Should we turn off this part of the task? An empty table doesn't seem useful to me.

Copy link
Contributor Author

@dbowling dbowling Jul 19, 2022

Choose a reason for hiding this comment

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

I couldn't find any other docs page that had an empty table, so this does stand out. Since it is generated dynamically, would disabling the rule have unexpected consequences in the future?

My thoughts now are to not disable the task, but instead alter aria-supported.js to output an empty string if this might bite us later. If not, disabling the task or culling the code would certainly be easiest.

Copy link
Contributor

Choose a reason for hiding this comment

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

Something needs to be done there. It's definitely outdated. Perhaps we should change that page to ARIA 1.3. I'll open a ticket.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll let you open a ticket and will mark this as resolved as-is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New issue for this is #3590.

"minami": "^1.2.3",
"mocha": "^9.2.0",
"node-notifier": "^8.0.1",
"mocha": "9.x",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not Mocha 10?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because version 10 drops IE 11 and Node 12 support.

https://github.com/mochajs/mocha/blob/master/CHANGELOG.md#1000--2022-05-01

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I spoke with @straker yesterday. Since we need the IE 11 support, we won't upgrade to Mocha 10 and will leave this comment as unresolved in the PR.

dbowling and others added 4 commits July 21, 2022 09:02
@dbowling dbowling force-pushed the chore/update-dependencies branch from 6da694d to 7a2c4d2 Compare July 22, 2022 16:17
@dbowling
Copy link
Contributor Author

This PR is ready for security review, even though it has pending changes--those will be left open, see above.

@dbowling dbowling requested a review from michael-siek July 25, 2022 19:10
@straker straker dismissed stale reviews from WilcoFiers, michael-siek, and themself July 25, 2022 19:55

Resolved

@straker straker merged commit d01532c into develop Jul 25, 2022
@straker straker deleted the chore/update-dependencies branch July 25, 2022 20:07
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.

5 participants