Skip to content

i18n: fix collect-strings on windows with pathToFileURL #14201

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
Merged
connorjclark merged 3 commits into from
Jul 11, 2022
Merged

i18n: fix collect-strings on windows with pathToFileURL #14201

connorjclark merged 3 commits into from
Jul 11, 2022

Conversation

@connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Jul 8, 2022

Fixes this error on master:

====
Collecting strings from C:\Users\cjamc\code\lighthouse/lighthouse-core
====
Collecting from lighthouse-core/audits/accessibility/accesskeys.js
Waiting for the debugger to disconnect...
node:internal/errors:465
    ErrorCaptureStackTrace(err);
    ^

Error [ERR_UNSUPPORTED_ESM_URL_SCHEME]: Only URLs with a scheme in: file, data are supported by the default ESM loader. On Windows, absolute paths must be valid file:// URLs. Received protocol 'c:'
    at new NodeError (node:internal/errors:372:5)
    at throwIfUnsupportedURLScheme (node:internal/modules/esm/resolve:1120:11)
    at defaultResolve (node:internal/modules/esm/resolve:1200:3)
    at ESMLoader.resolve (node:internal/modules/esm/loader:580:30)
    at ESMLoader.getModuleJob (node:internal/modules/esm/loader:294:18)
    at ESMLoader.import (node:internal/modules/esm/loader:380:22)
    at importModuleDynamically (node:internal/modules/esm/translators:106:35)
    at importModuleDynamicallyCallback (node:internal/process/esm_loader:35:14)
    at collectAllStringsInDir (file:///C:/Users/cjamc/code/lighthouse/lighthouse-core/scripts/i18n/collect-strings.js:555:24)
    at main (file:///C:/Users/cjamc/code/lighthouse/lighthouse-core/scripts/i18n/collect-strings.js:693:31) {
  code: 'ERR_UNSUPPORTED_ESM_URL_SCHEME'

@connorjclark connorjclark requested a review from a team as a code owner July 8, 2022 19:11
@connorjclark connorjclark requested review from adamraine and removed request for a team July 8, 2022 19:11
@brendankenny
Copy link
Contributor

brendankenny commented Jul 8, 2022

i18n: fix collect-strings on windows

why isn't this broken in CI? e.g. https://github.com/GoogleChrome/lighthouse/runs/7014418705?check_suite_focus=true#step:7:7 (which runs collect-strings)

@connorjclark
Copy link
Collaborator Author

connorjclark commented Jul 8, 2022

Can't say. I was using node 16. Can't try anything else, nvm is impossible for me to install.

@brendankenny
Copy link
Contributor

brendankenny commented Jul 8, 2022

Seems to work in Windows/Node 16 in GHA too: https://github.com/GoogleChrome/lighthouse/runs/7257743526?check_suite_focus=true#step:6:7

What's the reason for the stack packs change in foldersWithStrings? We're using / as path joins immediately above that line.

(also we define all those paths as absolute paths like `${LH_ROOT}/lighthouse-core` and then the first thing we do in collectAllStringsInDir is path.relative(LH_ROOT, dir) 🙄 this file could use some path rationalization :)

@connorjclark
Copy link
Collaborator Author

connorjclark commented Jul 8, 2022

On my system it ended up mixing forward slashes and backwards on that line. Probably the same for CI? Or not and that's the difference? Honestly I don't want to spend time debugging why it broke just on my windows machine right now, all I know is that this PR let's me run collect strings 🤷

@connorjclark
Copy link
Collaborator Author

connorjclark commented Jul 8, 2022 •
edited
Loading

....reverted the foldersWithStrings change. I guess I tried that first and forgot to undo it when I fixed the real issue, being lack of pathToFileURL

Still an open question why CI is fine but local wasn't. the path looks like

C:\Users\cjamc\code\lighthouse\lighthouse-core\audits\accessibility\accesskeys.js

which clearly is not a valid es module path. perhaps glob is somehow detecting a unix env in windows CI and giving us forward slash paths like /c/users/...?

or path.join is acting different in the two envs.

@connorjclark
Copy link
Collaborator Author

connorjclark commented Jul 8, 2022 •
edited
Loading

which clearly is not a valid es module path

sorry this isn't actually "clear". see testdouble/quibble#71 (comment)

it's wild, but paths in unix envs happens to be valid file:// es module paths because they are valid-ish URLs. not true for window paths.

@connorjclark connorjclark merged commit d13a919 into master Jul 11, 2022
@connorjclark connorjclark deleted the fix-col-strings-win branch July 11, 2022 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@adamraine adamraine adamraine approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@adamraine adamraine

Labels

waiting4reviewer

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@connorjclark @brendankenny @adamraine @devtools-bot