core(config): make module resolution async#13974
Conversation
| * @param {LH.Flags=} flags | ||
| */ | ||
| constructor(configJSON, flags) { | ||
| static async createConfigFromJson(configJSON, flags) { |
There was a problem hiding this comment.
Constructors can't be async, so this helper function exists now.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
|
||
| "target": "es2020", | ||
| "module": "es2020", | ||
| "module": "es2022", |
There was a problem hiding this comment.
oops, i used top level await in one place... let's get #13964 done first.
| import defaultConfig from '../config/default-config.js'; | ||
|
|
||
| const config = new Config(defaultConfig); | ||
| const config = await Config.createConfigFromJson(); |
There was a problem hiding this comment.
The defaultConfig is used by default.
| * @return {Config} | ||
| * @return {Promise<Config>} | ||
| */ | ||
| function generateConfig(configJson, flags) { |
There was a problem hiding this comment.
TODO for me: Make this FR config and add a generateLegacyConfig or something.
brendankenny
left a comment
There was a problem hiding this comment.
Far reaching but much cleaner than I was fearing. Nice one @connorjclark!
| * @param {LH.Flags=} flags | ||
| */ | ||
| constructor(configJSON, flags) { | ||
| static async fromJson(configJSON, flags) { |
There was a problem hiding this comment.
bikeshed: we call the type Config.Json but it's not necessarily json, and ideally the function name won't mislead. I don't have anything in particular I like instead...initialize() to piggyback on the existing FR name? create()? generate()? Plain old from()?
There was a problem hiding this comment.
I like initialize() but this code is deprecated now so 🤷
There was a problem hiding this comment.
The static function isn't deprecated, just the constructor. Outside of our tests there really isn't a reason you'd want to use the constructor (with this PR).
.fromX is a pattern that I know is common in dart (which supports multiple constructors), I wonder if that's also a pattern in other languages. so I'm leaning towards from. If we had unlimited characters fromUnresolved would be nice. but just from is ok too (and I'll add a note about the resolving bit in the comment)
| * @deprecated `Config.fromJson` should be used instead. | ||
| * @constructor | ||
| * @param {LH.Config.Json} configJSON | ||
| * @param {{settings: LH.Config.Settings, passes: ?LH.Config.Pass[], audits: ?LH.Config.AuditDefn[]}} opts |
There was a problem hiding this comment.
wow, ancient typing style
In preparation for converting
lighthouse-coreto esm (ref #12689), the config module resolution functions need to become async to accommodate dynamic imports. This PR maderequireWrapperasync, then made every calling function async.