Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix(config): fix faulty build output w/
--esm flag (#3404)
Stencil v2.16.0 introduced a regression, the symptoms of which are described here: #3403. Git bisect showed that this commit was the culprit: cca8951. What was the regression? ------------------------ What the user observed is that after upgrading from Stencil version 2.15.2 to version 2.16.0 the build output changed. With 2.15.2, when calling stencil like so: ``` stencil build --docs --dev --esm ``` the build output looked something like this: ``` dist ├── cjs ├── collection │ ├── components │ ├── test │ └── utils ├── esm │ └── polyfills ├── loader ├── stencil-one └── types ``` Whereas after upgrading to 2.16.0 the output looked something like this: ``` dist ├── stencil-one └── types ``` Why did the built output change? -------------------------------- Stencil previously explicitly supported a `--esm` boolean command-line flag. Before [the change to remove the legacy compiler](63595bc) this flag was used to control whether the output would be ES modules or not (I _think_). The flag was removed from the supported arguments list in [that same commit](63595bc#diff-e6a96bc13cd5c047a16a1888b27e88af52903e89848de68b0072d32ea11b1208L196), however some code that implicitly depended on the argument remained in the codebase, and it was eventually (partially) [added back to the codebase later on](#2339) but, although a change was made to `src/cli/parse-flags.ts` to support parsing the flag, it was *not* added to the `ConfigFlags` interface, which is intended to capture the names and types of all CLI arguments. Related to this history of the flag being added and removed from Stencil, on `src/compiler/config/validate-config.ts` there was, prior to the bad commit which introduced this regression, a call to the `setBooleanConfig` helper that looked like this: ```ts setBooleanConfig(config, 'buildDist', 'esm', !config.devMode || config.buildEs5); ``` This line creates an undocumented dependency on the `--esm` command-line flag, where it's presence or absence changes what the value of `config.buildDist` will be. This happens because of the implementation of `setBooleanConfig`, where if a value is passed for the third positional argument it will look at the command-line arguments passed to the Stencil CLI in order to find a value to set on the `Config` object. The relevant code looks like this: ```ts export const setBooleanConfig = <K extends keyof d.Config>( config: d.UnvalidatedConfig, configName: (K & keyof d.ConfigFlags) | K, flagName: keyof d.ConfigFlags | null, defaultValue: d.Config[K] ) => { if (flagName) { const flagValue = config.flags?.[flagName]; if (isBoolean(flagValue)) { config[configName] = flagValue; } } ``` The [commit that introduced the regression](cca8951#diff-53c120bef3205f89229fbe98a5aadfcb51012ead144729ade2fc597c1e9ad5e9R64) changed the above call to `setBooleanConfig` to this: ```ts setBooleanConfig(config, 'buildDist', null, !config.devMode || config.buildEs5); ``` The user observed the regression because they were using the `--esm` command-line flag with their project, so on any stencil version <= 2.15.2 the presence of that flag was causing `config.buildDist` to be set to `true`, resulting in the expected build output above. In 2.16.0 the behavior of the `--esm` flag was broken by change from `"esm"` to `null`. Why was the change introduced? ------------------------------ This change was introduced because `"esm"` was not listed as a supported command-line argument on the `ConfigFlags` interface and the third position argument to `setBooleanConfig` has type `keyof d.ConfigFlags`, so the change to `null` was in line with what `setBooleanConfig` expects for its arguments. The commit author (me!) was unsure of whether to make this change or not, and there was [some discussion on the PR](#3335 (comment)). Light testing did not turn up the regression because we weren't using the `--esm` flag. What is the fix? ---------------- The fix is to add `esm` to the `ConfigFlags` interface and to revert the bad line in `src/compiler/config/validate-config.ts`. This will allow anyone who is currently using the `--esm` flag to keep doing so. This fix does not deal with whether or not we want to continue supporting the `--esm` flag — it's possible that a larger refactor to how the `buildDist` variable is used throughout the codebase is in order — but doing this will unblock any users who are currently depending on Stencil's previous, undocumented behavior. I think this is the right fix for now, however, because an API change like this should not happen on a minor release, so the behavior of the compiler changing in this way should be viewed as a bug and we should take the steps to restore the old behavior. Any larger lessons learned? --------------------------- Part of the reason for the regression was that the CLI argument parsing code does not have a type level contract with the interface into which the CLI args are parsed. In particular, there are lists of supported CLI flags in `src/cli/parse-flags.ts` but these have _no connection_ with the `ConfigFlags` interface, which describes the shape of the object into which they are going to be shoved after their values are parsed. This means that although we have an interface that sort of _claims_, at least implicitly, because of how it's named, to enumerate the possible / allowed CLI arguments, we aren't actually utilizing the type system to enforce a tighter contract between the parsing code and that interface, so there is a possibility of the arguments declared in one or the other of these two places (the `parse-flags.ts` code or the `ConfigFlags` interface) drifting from the other. To address that root cause I think we should implement something to declare these properties in one place. For instance, we could declare a `ReadonlyArray` of Boolean flags, another of String flags, and then use types derived from those as keys for the `ConfigFlags` argument. Additionally, several of the commits in the history of the repository which had to do with this flag have no discussion, documentation, or context provided for the changes.
- Loading branch information
1 parent
30dc745
commit a847a6e