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

Upgrade aria-query to 5.3.0 and axobject-query to 3.2.1 #937

Merged
merged 1 commit into from
Aug 11, 2023

Conversation

jessebeach
Copy link
Collaborator

@jessebeach jessebeach commented Jun 14, 2023

The ARIA to HTML mapping 1.0 defines ARIA role to HTML element mappings explicitly. We did not have this sort of guidance in years past. The aria-query project was updated to reflect these mappings in A11yance/aria-query#447

This caused several tests to fail in this plugin. I've fixed them and upversioned aria-query and axobject-query.

@jessebeach jessebeach requested a review from ljharb June 14, 2023 04:11
@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Merging #937 (64bfea6) into main (1271153) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #937   +/-   ##
=======================================
  Coverage   99.29%   99.29%           
=======================================
  Files         104      104           
  Lines        1554     1555    +1     
  Branches      522      523    +1     
=======================================
+ Hits         1543     1544    +1     
  Misses         11       11           
Files Changed Coverage Δ
src/util/isNonInteractiveElement.js 100.00% <100.00%> (ø)

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Amazing, thank you!

@ljharb
Copy link
Member

ljharb commented Jun 14, 2023

hmm; looks like some tests are failing unrelated to the upgrade, due to eslint violations for older versions suddenly being out of order. I'll land this once I've resolved those.

dummdidumm added a commit to sveltejs/svelte that referenced this pull request Jun 19, 2023
closes #8728
Turns out all the removed previous test failures are indeed correct to be removed, according to the test adjustments in jsx-eslint/eslint-plugin-jsx-a11y#937
dummdidumm added a commit to sveltejs/svelte that referenced this pull request Jun 19, 2023
closes #8728
Turns out all the removed previous test failures are indeed correct to be removed, according to the test adjustments in jsx-eslint/eslint-plugin-jsx-a11y#937
@jessebeach jessebeach changed the title Upgrade aria-query to 5.2.1 and axobject-query to 3.2.1 Upgrade aria-query to 5.3.0 and axobject-query to 3.2.1 Jun 24, 2023
@khiga8
Copy link
Contributor

khiga8 commented Jul 17, 2023

Hi! One of our project has a dependency on the latest aria-query 5.3.0 and uses the latest eslint-plugin-jsx-a11y release (which we now understand can result in weirdness which we should be wary of). We noticed the no-interactive-element-to-noninteractive-role rule unexpectedly newly flags:

<td role="cell"/>

I believe this is a false positive that we'd want to avoid flagging. I noticed this PR bumps aria-query to 5.3.0 so it seemed relevant to comment on, but let me know if filing an issue is more appropriate.

@ljharb
Copy link
Member

ljharb commented Aug 9, 2023

@jessebeach any update?

@jessebeach
Copy link
Collaborator Author

@jessebeach any update?

@ljharb, you mentioned in a previous comment that you were going to look at unrelated failures. Have you had a chance to do that?

hmm; looks like some tests are failing unrelated to the upgrade, due to eslint violations for older versions suddenly being out of order. I'll land this once I've resolved those.

@ljharb
Copy link
Member

ljharb commented Aug 9, 2023

ah right, thanks, i'll follow up on that asap :-)

@ljharb ljharb merged commit 64bfea6 into main Aug 11, 2023
171 checks passed
@ljharb ljharb deleted the upgrade-aria-query-5.2.1 branch August 11, 2023 23:45
@ljharb
Copy link
Member

ljharb commented Jun 19, 2024

Turns out this was a breaking change (see A11yance/aria-query#497 (comment)) and will have to be reverted.

engines checks have been added to CI to ensure this doesn't regress in the future.

ljharb added a commit to ljharb/eslint-plugin-jsx-a11y that referenced this pull request Jun 19, 2024
@benmccann
Copy link

Or you could upgrade your project to stop using Node 4...

@ljharb
Copy link
Member

ljharb commented Jun 19, 2024

Sure, but that wouldn't unbreak this major line of the plugin, and it'd still need to be fixed.

@benmccann
Copy link

"need to be fixed" is a bit of a strong statement for an issue I can't imagine anyone would ever hit. Who the heck is still using Node 4 anymore?

@ljharb
Copy link
Member

ljharb commented Jun 19, 2024

semver doesn't care about actual breakage, it's defined by conceptual breakage.

that nobody has complained may mean that nobody's using node 4, for example - but it could just as likely mean that dequal already works on node 4 and the declaration is unnecessarily restrictive, or, that they're using a build process that masks syntax-related failures.

@benmccann
Copy link

And users don't care about conceptual breakage, but actual breakage. When the two options are that no one's using the affected version of Node or that version isn't even actually affected, this is an exercise in pedantry

@ljharb
Copy link
Member

ljharb commented Jun 20, 2024

I’m a user, and i care ¯_(ツ)_/¯ either way, releasing a v5 backport and a v6 with tighter engines seems like it’d address your needs - but also, why would a deep equal library need a feature that requires modern node? I still think fixing it in dequal is the easiest solution.

@benmccann
Copy link

I don't think I'd call Node 6 "modern node", but anyway I took a look at dequal and my guess is that it doesn't actually require Node 6 to run. However, all of its dev dependencies require either Node 6 or 8 to run, so the CI for the project tests only against Node 8. While it would probably be okay to expand or remove the version range in dequal's engines field, those older versions of Node would remain untested, so tbd whether the maintainer would want to support untested versions of Node

@ljharb
Copy link
Member

ljharb commented Jun 20, 2024

That would certainly be a limiting factor.

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.

4 participants