misc: import i18n messages as JSON modules #16500
Conversation
As far as I can tell, import attribute syntax should work everywhere lighthouse is supported. My intent behind doing this was to maybe get Lighthouse to the point where you can bundle it for NodeJS using something like esbuild or rollup. But it turns out that errors about these locales files were just the first ones to occur when executing a bundled lighthouse entrypoint :^). Audits and gatherers are still loaded completely dynamically from the filesystem. Maybe those could be dynamically imported instead, but that would be a more substantial change. Anyway, perhaps it's still better to ship this than to not. Fixes GoogleChrome#14611.
| import fs from 'fs'; | ||
|
|
||
| import {getModuleDirectory} from '../esm-utils.js'; | ||
| import ar from './locales/ar.json' with { type: 'json' }; |
There was a problem hiding this comment.
is there a more generic way to do this?
There was a problem hiding this comment.
I think @isker is referring to the ar import as a hardcoded locale (Arabic). To make this more generic, you could dynamically import the locale based on a variable like this:
const locale = 'ar'; // could be 'en', 'fr', etc. Or dynamically set this based on user preference
const messages = await import(`./locales/${locale}.json`, {
with: { type: 'json' }
});This way, you can support multiple languages without having to write a separate import for each one.
There was a problem hiding this comment.
I suspect dynamically importing them breaks the ability of a bundler to statically analyze the imports, which was the motivation behind me doing this in the first place.
Beyond that, I don't really understand why dynamic imports here are an improvement. What's the difference between writing down a list of locales and expanding them into dynamic imports, compared to writing down a list of static imports?
independent of the contents of this PR (I was the one that filed #14611 :), FWIW you can do this using the |
connorjclark
left a comment
connorjclark
left a comment
There was a problem hiding this comment.
Thanks!
I'm certain there's still more issues regarding bundling out-the-box - but as Brendan said, we do have a script to assist in that. It's what we use to bundle for DevTools and for PSI.
|
FYI this was a breaking change as import attributes require Node v20.10+ but Lighthouse currently supports v18.20. This file is imported in Node through:
|
|
hmm that does seem right ... need to figure out how our CI passed for 18.20 given that. I'll revert this change in the next release. Thanks for pointing it out. |
|
This says 18.20: https://nodejs.org/api/esm.html#import-attributes Could have sworn I also tested that import attributes work with node 18.20. |
|
I think you're right about the v18 version, but it's still a breaking change as you're saying you work with any version of Node v20. I did get a bit confused in thinking the min. was 20.10 as that's what MDN has but if you click on the little ... they're got v18.20 there too: 
The failing CI that brought this to my attention is using v20.9 so it's obvious why that failed - but in a perfect world engines should be something like |
|
I see. 20.9.0 was indeed the first release that the 20 line went LTS, so it is that exact single version that is broken, if the project disregards support for non-LTS versions (which is something I have just made up). I think this is a niche situation but I’ll leave it to the maintainers’ judgment. |
This reverts commit 4a84063.
This reverts commit 4a84063.
6 participants
As far as I can tell, import attribute syntax should work everywhere lighthouse is supported.
My intent behind doing this was to maybe get Lighthouse to the point where you can bundle it for NodeJS using something like esbuild or rollup. But it turns out that errors about these locales files were just the first ones to occur when executing a bundled lighthouse entrypoint :^). Audits and gatherers are still loaded completely dynamically from the filesystem. Maybe those could be dynamically imported instead, but that would be a more substantial change.
Anyway, perhaps it's still better to ship this than to not.
Fixes #14611.