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.

Sign up for GitHub

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

Jump to bottom

chore: upgrade aria-query and axobject-query #8724

Merged
merged 5 commits into from
Jun 14, 2023
Merged

chore: upgrade aria-query and axobject-query #8724

merged 5 commits into from
Jun 14, 2023

Conversation

benmccann
Copy link
Member

this upgrade removes a majority of Svelte's dependencies by replacing deep-equal with dequal in our dependency tree

@vercel
Copy link

vercel bot commented Jun 13, 2023

@benmccann is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

@gtm-nayan
Copy link
Contributor

Should be updated to 5.2.1, and I think we can probably get rid of util from the devDeps list and remove the relevant bits from the rollup config, so plugin-replace could possibly be removed as well.

ecosystem-ci should confirm whether it works or not through the eslint suites.

@benmccann
Copy link
Member Author

and I think we can probably get rid of util from the devDeps list and remove the relevant bits from the rollup config,

yes!! I forgot about those hacks. thanks for the reminder!

@gtm-nayan
Copy link
Contributor

gtm-nayan commented Jun 13, 2023 •
edited
Loading

There was another commit after 5.1.3 that updated the rules, so I think the test failures come from that instead, but also they don't quite look correct?

These warnings don't appear in the new version but it did previously. Is it correct? Maybe @geoffrich can take a look?

Here's the checks that have been removed

  -   {
  -     code: 'a11y-no-noninteractive-element-to-interactive-role',
  -     end: {
  -       column: 20,
  -       line: 49,
  -     },
  -     message: 'A11y: Non-interactive element <td> cannot have interactive role \'button\'',
  -     start: {
  -       column: 0,
  -       line: 49,
  -     },
  -   },
  -   {
  -     code: 'a11y-no-noninteractive-element-to-interactive-role',
  -     end: {
  -       column: 20,
  -       line: 21,
  -     },
  -     message: 'A11y: Non-interactive element <frame> cannot have interactive role \'row\'',
  -     start: {
  -       column: 0,
  -       line: 21,
  -     },
  -   },
  -   {
  -     code: 'a11y-no-noninteractive-element-to-interactive-role',
  -     end: {
  -       column: 57,
  -       line: 5,
  -     },
  -     message: 'A11y: Non-interactive element <body> cannot have interactive role \'combobox\'',
  -     start: {
  -       column: 0,
  -       line: 5,
  -     },
  -   },
  -   {
  -     code: 'a11y-no-interactive-element-to-noninteractive-role',
  -     end: {
  -       column: 27,
  -       line: 146,
  -     },
  -     message: 'A11y: <summary> cannot have role \'listitem\'',
  -     start: {
  -       column: 0,
  -       line: 146,
  -     },
  -   },

@benmccann
Copy link
Member Author

benmccann commented Jun 13, 2023 •
edited
Loading

For reference, here's one of the files the warnings are stored in to compare against:

svelte/packages/svelte/test/validator/samples/a11y-no-noninteractive-element-to-interactive-role/warnings.json

Line 572 in 8bd9021

"message": "A11y: Non-interactive element <td> cannot have interactive role 'button'",

And the file printing the warning:

svelte/packages/svelte/src/compiler/compile/nodes/Element.js

Line 758 in 8bd9021

// no-interactive-element-to-noninteractive-role

Which is powered from utils/a11y.js:

svelte/packages/svelte/src/compiler/compile/utils/a11y.js

Line 215 in 8bd9021

export function is_interactive_element(tag_name, attribute_map) {

@geoffrich
Copy link
Member

Looking at the test failures, we're now missing four warnings:

  • summary cannot have listitem
  • body cannot have combobox
  • frame cannot have row
  • td cannot have button

I don't have a good enough understanding of the aria-query package to understand whether that was an intentional change or not -- briefly looked through the PR that changed things and there's a lot going on.

These seem edge-casey enough to me that I don't think they should hold up merging this dependency update. Maybe temporarily remove them from the warnings.json so the tests pass and create a followup ticket for me to investigate further?

@gtm-nayan
Copy link
Contributor

/ecosystem-ci run eslint-plugin-svelte

@svelte-ecoystem-ci
Copy link

svelte-ecoystem-ci bot commented Jun 13, 2023 •
edited
Loading

📝 Ran ecosystem CI: Open

suite result
eslint-plugin-svelte ❌ failure

@gtm-nayan
Copy link
Contributor

Ahh right, that's a thing for now.

@gtm-nayan
Copy link
Contributor

/ecosystem-ci run prettier-plugin-svelte

@svelte-ecoystem-ci
Copy link

svelte-ecoystem-ci bot commented Jun 13, 2023 •
edited
Loading

📝 Ran ecosystem CI: Open

suite result
prettier-plugin-svelte ✅ success

@benmccann
Copy link
Member Author

create a followup ticket for me to investigate further

@geoffrich I did a fair amount of debugging and created a ticket here: #8728. I pretty clearly see what changed in the aria-query package, but I'm not familiar enough with the a11y code to know if there's an issue or what might need to change

@vercel
Copy link

vercel bot commented Jun 13, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
svelte-dev-2 ❌ Failed (Inspect) Jun 13, 2023 4:22pm

@benmccann benmccann added this to the 4.x milestone Jun 13, 2023
@benmccann benmccann mentioned this pull request Jun 14, 2023
Closed
@benmccann benmccann merged commit 83e9178 into sveltejs:version-4 Jun 14, 2023
@benmccann benmccann deleted the aria-query branch June 14, 2023 15:23
@gtm-nayan gtm-nayan mentioned this pull request Jun 17, 2023
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gtm-nayan gtm-nayan gtm-nayan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@benmccann @gtm-nayan @geoffrich