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

deps: update axe-core to 3.4.1 #10056

Merged
merged 2 commits into from
Jan 28, 2020
Merged

deps: update axe-core to 3.4.1 #10056

merged 2 commits into from
Jan 28, 2020

Conversation

jayaddison
Copy link
Contributor

@jayaddison jayaddison commented Nov 30, 2019

Summary
Updates axe-core==3.4.1 following its release on 2019-12-18 (release tag).

@jayaddison
Copy link
Contributor Author

Upgrading the dependency here pushes the lighthouse-dt-bundle.js file size over the configured limit. I'm not sure what the policy on increasing that is, so I'll leave the PR as-is.

Error snippets from the build log:

 FAIL  ./dist/lighthouse-dt-bundle.js: 401.52KB > maxSize 400KB (gzip)
...
The command "yarn bundlesize" exited with 1.

@jayaddison
Copy link
Contributor Author

@brendankenny @patrickhulce From the test failures in this PR, upgrading axe-core to 3.4.0 pushes the lighthouse devtools bundle over the configured size limit of 400KB.

I'm not going to push any commits which change build limits, so was wondering if there's a good way to resolve the failure in order to unblock the version bump?

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

Thanks very much for the PR @jayaddison!

Hm, that's a fairly substantial increase for 3.4.0, an extra ~60KB minified script (13KB gzipped) :/

image

Is there something in this release particularly compelling to warrant the increase? If not, I'd be inclined to skip and see if we can require in smaller pieces or work to find some bundle size wins in axe-core for the next version.

Just noting we also need to mark aria-roledescription as unused or create an audit for it before landing.

@jayaddison
Copy link
Contributor Author

@patrickhulce Thanks for the quick response!

For the use case I'm looking at, it fixes an accessibility false-positive (dequelabs/axe-core#1688).

The aria spec considers a combobox having a child with input[type=search] as valid, whereas 3.3.0 marks a failure in that situation. The select2 library renders DOM elements which fall into the failure case.

I hadn't noticed that the size increase was so significant; let me dig around and see if I can figure out what happened there.

@jayaddison
Copy link
Contributor Author

@patrickhulce It looks like the majority of the bundle size increase is from locale data, much of which is new in 3.4.0. From a slightly unscientific dir-by-dir diff and sort:

cat foo | sort
node_modules/axe-core/axe.d.ts 16K
node_modules/axe-core/axe.d.ts 6K
node_modules/axe-core/axe.js 644K
node_modules/axe-core/axe.js 784K
node_modules/axe-core/axe.min.js 344K
node_modules/axe-core/axe.min.js 404K
...
node_modules/axe-core/CHANGELOG.md 76K
node_modules/axe-core/CHANGELOG.md 88K
...
node_modules/axe-core/doc 584K
node_modules/axe-core/doc 608K
node_modules/axe-core/lib 6.3M
node_modules/axe-core/lib 6.4M
...
node_modules/axe-core/locales 148K
node_modules/axe-core/locales 244K
...

(identical file sizes omitted)

Let me see if the additional locale data would benefit lighthouse; if not, perhaps it can be excluded somehow.

@jayaddison
Copy link
Contributor Author

@patrickhulce FWIW, another line of possibility is a change in one of axe-core's dependencies:

https://github.com/dequelabs/axe-core/blob/v3.3.0/package.json#L89
vs
https://github.com/dequelabs/axe-core/blob/v3.4.0/package.json#L82
as per dequelabs/axe-core#1707

This seems to result in a good chunk of extra code (basically an inlined import) in the output axe.js.

@jayaddison
Copy link
Contributor Author

(didn't find anything conclusive on either of those, so some git bisect is happening)

@jayaddison
Copy link
Contributor Author

Ok, my mistake earlier - in fact it is dequelabs/axe-core#1829 that lead to the marked size increase in axe-core which then resulted in the overall bundle size failures here.

I'm still doing a little more investigation into the precise cause within that PR, and the value of the change to lighthouse, which may take a little more time given that I'm a bit new to each of them.

@jayaddison
Copy link
Contributor Author

@patrickhulce Alright, yep - I've confirmed locally that it's the introduction of core-js in axe-core (which in turn was necessary for some array functionality polyfilling - cc @straker) that led to the majority of the size increase observed here.

Taking some time out here, but glad to help with any further investigation.

@patrickhulce
Copy link
Collaborator

FWIW I don't think 60KB is a dealbreaker, I just want to make sure we're getting something out of this tradeoff. The higher priority issue IMO is the now never-failing audit, really appreciate the investigation though we'd always like to help axe folks out with perf when we can :)

FYI @exterkamp apparently axe has a decent amount of locale data these days we might want to figure out how to leverage in our i18n'ing efforts.

@jayaddison
Copy link
Contributor Author

@patrickhulce All makes sense, yep. I think incomplete may be a relatively new concept (~late 2017), and implies neither a clear pass nor fail (docs).

IIRC sometimes lighthouse displays grayed out advisory items which need to be checked manually? If it's reasonably possible to transform the multiple-labels check into one of those, that might be an option (I'd probably open it as a PR branching off from here if so).

I didn't find any functional references to the incomplete return code in lighthouse right now, so I assume this would be a new code path if so.

@straker
Copy link

straker commented Dec 6, 2019

@jayaddison Incomplete can also mean the rule threw an error, in which case we report the error in the message of the incomplete result (using the properties error, errorNode, message, and stack).

@jayaddison
Copy link
Contributor Author

@straker Neat, thanks for the context. Sounds like there's larger scope for comprehensively handling incomplete results, so as a new committer to lighthouse, I might politely bow out of designing & handling those (unless there are any small changes I can help with to improve things).

@patrickhulce
Copy link
Collaborator

patrickhulce commented Jan 27, 2020

alright this should be unblocked now with #10072 merging! a simple rebase and we should be good to go

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

LGTM, thanks so much for sticking with this @jayaddison I know it was a slog! 🎉 💯

@jayaddison
Copy link
Contributor Author

@patrickhulce Sometimes felt like I've been aiming for a record commit-to-line-diff ratio :)

Just one more yarn type-check issue to resolve which is showing up in master too I think:

lighthouse-core/audits/accessibility/axe-audit.js(51,7): error TS2322: Type 'Result' is not assignable to type 'Product'.

Looking into this currently.

@jayaddison
Copy link
Contributor Author

I believe #10270 should resolve the type check failures. I'm leaning on CI a little, having run some but not all tests locally.

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

Successfully merging this pull request may close these issues.

6 participants