-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
i18n: fix collect-strings on windows with pathToFileURL #14201
Conversation
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) |
Can't say. I was using node 16. Can't try anything else, nvm is impossible for me to install. |
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 (also we define all those paths as absolute paths like |
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 🤷 |
....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
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 or path.join is acting different in the two envs. |
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. |
Fixes this error on master: