-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
core(hreflang): remove eval, import axe valid-langs.js directly #13385
Conversation
oh no |
|
||
echo '// @ts-nocheck' > "$OUT_FILE" | ||
echo '// Auto-generated by lighthouse-core/scripts/copy-axe-valid-langs.sh' >> "$OUT_FILE" | ||
sed 's/Sting/string/' "$LH_ROOT_DIR"/node_modules/axe-core/lib/core/utils/valid-langs.js >> "$OUT_FILE" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sed 's/Sting/string/' "$LH_ROOT_DIR"/node_modules/axe-core/lib/core/utils/valid-langs.js >> "$OUT_FILE" | |
sed 's/String/string/' "$LH_ROOT_DIR"/node_modules/axe-core/lib/core/utils/valid-langs.js >> "$OUT_FILE" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they fixed it so it is String
now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should talk to the axe folks about putting a package.json in e.g. the lib/
directory so it doesn't interfere with their main
but allows importing modules under lib/
?
.github/workflows/ci.yml
Outdated
@@ -50,6 +50,7 @@ jobs: | |||
- run: yarn i18n:checks | |||
- run: yarn dogfood-lhci | |||
- run: bash lighthouse-core/scripts/copy-util-commonjs.sh | |||
- run: bash lighthouse-core/scripts/copy-axe-valid-langs.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we skip introducing machinery to update? The list of langs hasn't changed since March 2018. The third-party directory needs a readme anyways, we could just put update instructions in there too. Ideally long term we just get the file to be importable from the node_modules/ dependency.
* @return {boolean} | ||
*/ | ||
function isExpectedLanguageCode(hreflang) { | ||
function isExpectedLanguageCode(hreflang, isValidLang) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to make this a callback? Could import where needed:
function isExpectedLanguageCode(hreflang, isValidLang) { | |
async function isExpectedLanguageCode(hreflang) { | |
// TODO(esmodules): use static import when file is esm. | |
const isValidLang = (await import('../../../third-party/axe/valid-langs.js')).default; | |
// ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to limit the async from going where it made less sense.
third-party/axe/package.json
Outdated
@@ -0,0 +1,4 @@ | |||
{ | |||
"type": "module", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
directory needs a license/readme (or metadata but readme is fine, honestly): go/thirdparty/non-google3
Get this error when running the integration recipe test: I suppose this is related to jest, although apparently jest has supported dynamic imports for over a year now jestjs/jest#10620 |
@@ -12,7 +12,6 @@ docs/ | |||
lighthouse-core/lib/sd-validation/ | |||
lighthouse-core/scripts/* | |||
lighthouse-core/test/ | |||
lighthouse-core/third_party/src/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drive by: this is not a folder
third-party/axe/package.json
Outdated
@@ -0,0 +1,4 @@ | |||
{ | |||
"license": "MPL-2.0", | |||
"//": "Any directory that uses `import ... from` or `export ...` must be type module. Temporary file until root package.json is type: module" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no "type": "module"
anymore, should this comment refer to the license or can we remove this file?
*/ | ||
static audit({LinkElements}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need to be async anymore
|
||
echo '// @ts-nocheck' > "$OUT_FILE" | ||
echo '// Auto-generated by lighthouse-core/scripts/copy-axe-valid-langs.sh' >> "$OUT_FILE" | ||
sed 's/Sting/string/' "$LH_ROOT_DIR"/node_modules/axe-core/lib/core/utils/valid-langs.js >> "$OUT_FILE" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they fixed it so it is String
now
Fixes #13205