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

Remove usage of eval in hreflang.js #13205

Closed
connorjclark opened this issue Oct 12, 2021 · 5 comments · Fixed by #13385
Closed

Remove usage of eval in hreflang.js #13205

connorjclark opened this issue Oct 12, 2021 · 5 comments · Fixed by #13385
Assignees

Comments

@connorjclark
Copy link
Collaborator

see: https://docs.google.com/document/d/10gp-4xzsQuJpdfMdYHPZbf6EYq3lBnu1Adc11bcMQE8/edit#

CDT wants to tighten their CSP, and this came up as an issue. We could revert what #11442 did–alternatively we could add the list of locales to the Accessibility artifact.

@brendankenny
Copy link
Member

brendankenny commented Oct 12, 2021

#11661 (comment) may finally be able to shine :)

@paulirish
Copy link
Member

Ohh also I think we didnt catch that the axe list of langs went from 20kb to 2kb in axe 4.1.0 (which we've had since #11661

dequelabs/axe-core#2527
https://github.com/dequelabs/axe-core/blob/develop/lib/core/utils/valid-langs.js

nice job axe folks :D

@connorjclark
Copy link
Collaborator Author

No, definitely caught that, it was on my suggestion :)

@brendankenny
Copy link
Member

brendankenny commented Oct 13, 2021

we're dropping support for Node 12 (and we decided to move past Node 12.17 since last December anyways), so dynamic import for hreflang (and reverting back to the old fs.readFileSync in the accessibility gatherer) would maintain the size savings and is easily doable for LH9

@connorjclark
Copy link
Collaborator Author

wouldn't that inline as a string (brfs) that module and include it in the bundle as JS (for import)?

I don't see how dynamic import is any different than the old commonjs require for that explicit module, in terms of avoiding the duplication

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

Successfully merging a pull request may close this issue.

3 participants