test(config): remove strictNullChecks errors from compiler/config #3335

alicewriteswrongs · 2022-04-15T16:42:23Z

This is another PR for removing some strictNullChecks errors, in
particular in the code related to loading and validating the Config.

This PR proposes a change to how the Config loading / validation
pipeline works. Instead of dealing the whole way through with a single
Config type, this puts a boundary in place which is crossed in the
validateConfig function between an UnvalidatedConfig and a Config.

How does this work? Basically Config is the type that we expect to be
able to pass around everywhere else in the codebase without issue, while
UnvalidatedConfig is a type which is around to specifically denote the
fact that the object we're dealing with is sort of a 'config in
progress' and it cannot yet be used freely throughout the compiler.

UnvalidatedConfig is implemented w/ a new type I added called Loose,
which looks like this:

type Loose<T extends Object> = Record<string, any> & Partial<T>

UnvalidatedConfig looks like this:

type UnvalidatedConfig = Loose<Config>

This amounts to 1) making all properties on Config optional (via
Partial) and 2) allowing access to properties which are not defined
on Config.

We need the former because this opens the door to later changing the
typing of properties on Config to not all be optional (as they
currently are). This will be a big help towards turning
strictNullChecks on, since doing so right now results in a literal ton
of errors around property access on Config objects—consider, for
instance, that currently Config.sys is an optional property, so every
time the .sys property is accessed we'll get an error with
strictNullChecks.

We need the latter change because with strictNullChecks if you do
something like the following you'll get a type error:

interface Foo {
    bar: string
}

let obj: Foo = {
    bar: "hey!"
}

let otherPropValue = obj["asdfasdf"]

TypeScript here will (and should!) throw an error for otherPropValue
because the index type on Foo is "bar" and not string. The
Record<string, any> & bit in the definition of Loose lets us access
properties not defined on Config in our validation code (for an
example of this see the setBoolean function in
/src/compiler/config/config-utils.ts) without giving up on types in
Config entirely. What do I mean by that? Basically just that if you do
this:

let config: UnvalidatedConfig = someFunctionThatProducesConfig();

then the type of config["somePropertyWeDontDefine"] will be any, and
we can do things we need to with such properties within the validation
code, but if I access config.sys the type will still be
CompilerSystem. So for this reason I think typing something as
Loose<T> is superior to just doing Record<string, any> or Object
or (shudders) any.

This commit just gets us started with this sort of 'lifecycle' for
Config objects (they start out as UnvalidatedConfigs, get passed
through validateConfig and come out as full Config objects) and more
work will be needed to get all of the config validation and loading code
on board. Once we do that we can safely start removing optional
properties on Config itself, since we'll have assurance from the
compiler that anything marked Config has already been fully validated
and should always have, e.g., a .sys property, a buildDir property,
etc. This will make it much easier for us to turn strictNullChecks on
without having to sprinkle the codebase with (literally hundreds of)
optional property accesses (?.).

Pull request checklist

Please check if your PR fulfills the following requirements:

Tests for the changes have been added (for bug fixes / features)
Docs have been reviewed and added / updated if needed (for bug fixes / features)
Build (npm run build) was run locally and any changes were pushed
Unit tests (npm test) were run locally and passed
E2E Tests (npm run test.karma.prod) were run locally and passed
Prettier (npm run prettier) was run locally and passed

Pull request type

Please check the type of change your PR introduces:

What is the current behavior?

I was working a bit yesterday and today on what would be involved in turning on strictNullChecks for typescript. I noticed that the Config object in particular is not in a good position to comply with strictNullChecks, since almost every property defined on it is optional. This means that w/ strictNullChecks almost every time we access a property on it we'd need to add a ?. or if (config.sys === undefined) or something like that.

What is the new behavior?

I am proposing here a change to how this object is typed, specifically by adding a new type called UnvalidatedConfig that we use throughout the configuration loading / validation process. Then our validateConfig function has it's signature changed, from

function validateConfig(config: Config): Config

to

function validateConfig(confing: UnvalidatedConfig): Config

This will give us some confidence that any time we see a Config we know it's already good to go, and we could start to remove all the optional typings on it so that we'd be able to turn strictNullChecks on.

This change also brings us from 2229 strictNullChecks errors on main to 2185 errors, which isn't a ton but I think introducing the UnvalidatedConfig change will allow us to more strictly type the Config object in the future and that will I think facilitate getting rid of a lot more errors.

Does this introduce a breaking change?

Yes
No

Testing

I ran the typechecking a bunch, ran the tests a whole bunch, and tried it out in a local test component I have. I am fairly confident that these changes are basically confined to the type system and changing null to undefined in a few places, but it's definitely something to check out thoroughly!

Other information

src/compiler/config/config-utils.ts

src/compiler/config/validate-config.ts

src/compiler/config/validate-hydrated.ts

src/compiler/config/config-utils.ts

src/declarations/stencil-public-compiler.ts

src/compiler/config/config-utils.ts

src/compiler/config/validate-dev-server.ts

rwaskiewicz · 2022-04-25T13:29:56Z

src/compiler/config/validate-dev-server.ts

  const addressSplit = devServer.address.split(':');
  if (addressSplit.length > 1) {
    if (!isNaN(addressSplit[1] as any)) {
      devServer.address = addressSplit[0];
-      addressPort = parseInt(addressSplit[1], 10);
+      devServer.port = parseInt(addressSplit[1], 10);


Is it possible that we're accidentally prematurely overwriting this value if flags.port is a number. It looks like if the previous implementation, addressSplit[1] took precedence over flags.port. Here, it looks like we've flipped that

I think you're right - I moved the if (isNumber(flags?.port)) { ... } block up above this addressSplit business so that we'll start with flags.port and then overwrite it with addressSplit[1] if it makes sense to do so

I think this can still be unintentionally overwritten.

In the scenario where we have:

{ devServer: { port: 8080, address: google.com:9999 } }

In the previous version:

'9999' would be stored in addressPort (line)

devServer.port would be set to '8080' (line)

this if/else if would stop us from using addressPort further, so we'd end up with devServer.port being '8080'

In the new version:

devServer.port would be set to '8080' (line)

devServer.port would be set to '9999' (line)

ok I went and re-did both the logic here and then also added a bunch of tests that passed with the original behavior (I filled in a few gaps with testing too, and de-duped some identical tests using it.each while I was in there).

Hopefully the new version 1) keeps the same behavior and 2) is easier to read and follow!

rwaskiewicz · 2022-04-25T13:32:20Z

src/compiler/config/validate-dev-server.ts

  }

  const flags = config.flags;
  const devServer = { ...config.devServer };

-  if (isString(flags.address)) {
+  if (flags && flags.address && isString(flags.address)) {


I wonder if it makes sense to update the declaration of flags on line 14 to default to an object literal like so:

- const flags = config.flags; + const flags = config.flags || {};

That way, we don't need to do checks such as this:

Suggested change

if (flags && flags.address && isString(flags.address)) {

if (flags.address && isString(flags.address)) {

or do optional chaining on every usage of flags throughout this function.

Thoughts?

I think that's sensible!

rwaskiewicz · 2022-04-25T13:39:20Z

src/compiler/config/validate-testing.ts

  const testing = (config.testing = Object.assign({}, config.testing || {}));

  if (!config.flags || (!config.flags.e2e && !config.flags.spec)) {
    return;
  }

-  let configPathDir = config.configPath;
+  let configPathDir = config.configPath!;


For my own understanding, how do we know that configPath exists on config at this point in the function?

Basically we don't (right now) have a type-level assurance that it's not null, but if you look at the loadConfig function you'll see that we basically try to read in the configuration file at configPath before we call the validate-config function with the output, and there is an early return if something goes wrong with reading the config. So basically by the time we're calling validate-config (and then subsequently validate-testing) we already know that this is present because we've been able to read a file at this path.

This is my understanding of how this works anyway!

Ah, that makes sense to me. I think this is fine in that case. We may want to think about 'semi-validated' configs in the future, but I don't want to create a ton of types that are PartialConfig.*s just yet 😆

rwaskiewicz · 2022-04-25T13:43:16Z

src/utils/helpers.ts

@@ -82,10 +82,11 @@ export const pluck = (obj: { [key: string]: any }, keys: string[]) => {

 export const isBoolean = (v: any): v is boolean => typeof v === 'boolean';
 export const isDefined = (v: any) => v !== null && v !== undefined;
-export const isUndefined = (v: any) => v === null || v === undefined;
+export const isUndefined = (v: any): v is null | undefined => v === null || v === undefined;
 export const isFunction = (v: any): v is Function => typeof v === 'function';
 export const isNumber = (v: any): v is boolean => typeof v === 'number';


Not related to this PR, but I think there's a tiny typo in this guard:

Suggested change

export const isNumber = (v: any): v is boolean => typeof v === 'number';

export const isNumber = (v: any): v is number => typeof v === 'number';

alicewriteswrongs · 2022-04-25T14:19:36Z

@rwaskiewicz one higher-level / "conceptual" question I was wondering about after your comment here - do we want to have a Config type with all the optional properties, and have validateConfig have a signature like Loose<Config> -> ValidatedConfig where ValidatedConfig is maybe for right now basically type ValidatedConfig = Config but could eventually become something like type ValidatedConfig = Required<Config>?

Right now in this PR, which I put together without reflecting super deeply on this stuff I have added an UnvalidatedConfig type which is Loose<Config>. It doesn't seem to me like we'd want to have Stencil users be exposed to an UnvalidatedConfig type since that would be a weird and confusing change from how they've been using the Config type until now - which makes me lean towards introducing a ValidatedConfig type instead which is only used internally within the compiler.

I'm not sure - what are your thoughts?

rwaskiewicz

Looking good! Mostly just a few optional chaining comments

src/compiler/config/config-utils.ts

src/compiler/config/validate-dev-server.ts

src/compiler/config/validate-config.ts

src/compiler/config/config-utils.ts

rwaskiewicz · 2022-04-28T13:13:05Z

src/compiler/config/validate-config.ts

@@ -48,7 +61,7 @@ export const validateConfig = (userConfig?: Config) => {
  setBooleanConfig(config, 'sourceMap', null, typeof config.sourceMap === 'undefined' ? false : config.sourceMap);
  setBooleanConfig(config, 'watch', 'watch', false);
  setBooleanConfig(config, 'buildDocs', 'docs', !config.devMode);
-  setBooleanConfig(config, 'buildDist', 'esm', !config.devMode || config.buildEs5);
+  setBooleanConfig(config, 'buildDist', null, !config.devMode || config.buildEs5);


Hmmm looks like this line (with the flagName set to 'esm' was added in a big refactor commit. I can't find a case where we use 'esm' as a property accessor on a config object directly (although that doesn't cover something like config[someVarWhoseValueIsStringEsm]. I smoke tested this against the output of create-stencil and couldn't see anything immediately obvious.

My vote is we keep this change as is, and if we need to back it out we'll do just that

rwaskiewicz · 2022-04-28T13:23:31Z

src/compiler/config/validate-dev-server.ts

  const addressSplit = devServer.address.split(':');
  if (addressSplit.length > 1) {
    if (!isNaN(addressSplit[1] as any)) {
      devServer.address = addressSplit[0];
-      addressPort = parseInt(addressSplit[1], 10);
+      devServer.port = parseInt(addressSplit[1], 10);


I think this can still be unintentionally overwritten.

In the scenario where we have:

{ devServer: { port: 8080, address: google.com:9999 } }

In the previous version:

'9999' would be stored in addressPort (line)

devServer.port would be set to '8080' (line)

this if/else if would stop us from using addressPort further, so we'd end up with devServer.port being '8080'

In the new version:

devServer.port would be set to '8080' (line)

devServer.port would be set to '9999' (line)

rwaskiewicz · 2022-04-28T13:26:21Z

src/compiler/config/validate-testing.ts

  const testing = (config.testing = Object.assign({}, config.testing || {}));

  if (!config.flags || (!config.flags.e2e && !config.flags.spec)) {
    return;
  }

-  let configPathDir = config.configPath;
+  let configPathDir = config.configPath!;


Ah, that makes sense to me. I think this is fine in that case. We may want to think about 'semi-validated' configs in the future, but I don't want to create a ton of types that are PartialConfig.*s just yet 😆

alicewriteswrongs · 2022-05-10T16:49:47Z

@rwaskiewicz I think comments should all be addressed and this is ready for another review now (I updated it last week but forgot to ping you — sorry!)

src/compiler/config/test/validate-dev-server.spec.ts

This is another PR for removing _some_ strictNullChecks errors, in particular in the code related to loading and validating the `Config`. This PR proposes a change to how the `Config` loading / validation pipeline works. Instead of dealing the whole way through with a single `Config` type, this puts a boundary in place which is crossed in the `validateConfig` function between an `UnvalidatedConfig` and a `Config`. How does this work? Basically `Config` is the type that we expect to be able to pass around everywhere else in the codebase without issue, while `UnvalidatedConfig` is a type which is around to specifically denote the fact that the object we're dealing with is sort of a 'config in progress' and it cannot yet be used freely throughout the compiler. `UnvalidatedConfig` is implemented w/ a new type I added called `Loose`, which looks like this: ```ts type Loose<T extends Object> = Record<string, any> & Partial<T> ``` `UnvalidatedConfig` looks like this: ```ts type UnvalidatedConfig = Loose<Config> ``` This amounts to 1) making all properties on `Config` optional (via `Partial`) and 2) allowing access to properties which are _not_ defined on `Config`. We need the former because this opens the door to later changing the typing of properties on `Config` to _not_ all be optional (as they currently are). This will be a big help towards turning `strictNullChecks` on, since doing so right now results in a literal ton of errors around property access on `Config` objects—consider, for instance, that currently `Config.sys` is an optional property, so _every time_ the `.sys` property is accessed we'll get an error with `strictNullChecks`. We need the latter change because with `strictNullChecks` if you do something like the following you'll get a type error: ```ts interface Foo { bar: string } let obj: Foo = { bar: "hey!" } let otherPropValue = obj["asdfasdf"] ``` TypeScript here will (and should!) throw an error for `otherPropValue` because the index type on `Foo` is `"bar"` and not `string`. The `Record<string, any> &` bit in the definition of `Loose` lets us access properties not defined on `Config` in our validation code (for an example of this see the `setBoolean` function in `/src/compiler/config/config-utils.ts`) without giving up on types in `Config` entirely. What do I mean by that? Basically just that if you do this: ```ts let config: UnvalidatedConfig = someFunctionThatProducesConfig(); ``` then the type of `config["somePropertyWeDontDefine"]` will be `any`, and we can do things we need to with such properties within the validation code, _but_ if I access `config.sys` the type will still be `CompilerSystem`. So for this reason I think typing something as `Loose<T>` is superior to just doing `Record<string, any>` or `Object` or (shudders) `any`. This commit just gets us started with this sort of 'lifecycle' for Config objects (they start out as `UnvalidatedConfig`s, get passed through `validateConfig` and come out as full `Config` objects) and more work will be needed to get all of the config validation and loading code on board. Once we do that we can safely start removing optional properties on `Config` itself, since we'll have assurance from the compiler that anything marked `Config` has already been fully validated and should always have, e.g., a `.sys` property, a `buildDir` property, etc. This will make it much easier for us to turn `strictNullChecks` on without having to sprinkle the codebase with (literally hundreds of) optional property accesses (`?.`).

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 ESModules 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.

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.

alicewriteswrongs requested a review from a team April 15, 2022 16:42

alicewriteswrongs force-pushed the ap/config-null-checks branch 4 times, most recently from 21272d7 to 9b5b953 Compare April 15, 2022 19:35

alicewriteswrongs commented Apr 15, 2022

View reviewed changes

src/compiler/config/config-utils.ts Show resolved Hide resolved

src/compiler/config/config-utils.ts Show resolved Hide resolved

src/compiler/config/validate-config.ts Outdated Show resolved Hide resolved

src/compiler/config/validate-hydrated.ts Show resolved Hide resolved

alicewriteswrongs force-pushed the ap/config-null-checks branch from 9b5b953 to 9266dda Compare April 22, 2022 17:46

rwaskiewicz requested changes Apr 25, 2022

View reviewed changes

alicewriteswrongs force-pushed the ap/config-null-checks branch 2 times, most recently from 97ca503 to c395b49 Compare April 27, 2022 20:37

rwaskiewicz requested changes Apr 28, 2022

View reviewed changes

alicewriteswrongs force-pushed the ap/config-null-checks branch from adb3629 to fe90371 Compare April 29, 2022 16:26

rwaskiewicz approved these changes May 10, 2022

View reviewed changes

src/compiler/config/test/validate-dev-server.spec.ts Show resolved Hide resolved

alicewriteswrongs added 11 commits May 10, 2022 15:27

remove the as cast

4f64b8a

fix some typings on those is${TypeName} functions

f748d28

declare a flags if none

d42d9b7

responding to feedback

8b8b314

addressing some more feedback

ab9f680

more feedback

fd2dbbc

some feedback changes

2a9ec5b

temporarily put back the logic from main in validate-dev-server.ts

85acf7b

update logic for port, address, etc in validate-dev-server

9c536ed

fix issue causing karma tests to fail

fba2ade

alicewriteswrongs force-pushed the ap/config-null-checks branch from 68d0ffb to fba2ade Compare May 10, 2022 20:03

rwaskiewicz approved these changes May 12, 2022

View reviewed changes

Merge branch 'main' into ap/config-null-checks

c724aff

rwaskiewicz merged commit cca8951 into main May 12, 2022

rwaskiewicz deleted the ap/config-null-checks branch May 12, 2022 13:17

alicewriteswrongs mentioned this pull request Jun 2, 2022

fix(config): fix faulty build output w/ --esm flag #3404

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(config): remove strictNullChecks errors from compiler/config #3335

test(config): remove strictNullChecks errors from compiler/config #3335

alicewriteswrongs commented Apr 15, 2022 •

edited

Loading

rwaskiewicz Apr 25, 2022

alicewriteswrongs Apr 27, 2022

rwaskiewicz Apr 28, 2022

alicewriteswrongs Apr 29, 2022

rwaskiewicz Apr 25, 2022

alicewriteswrongs Apr 25, 2022

rwaskiewicz Apr 25, 2022

alicewriteswrongs Apr 25, 2022

rwaskiewicz Apr 28, 2022

rwaskiewicz Apr 25, 2022

alicewriteswrongs Apr 25, 2022

alicewriteswrongs commented Apr 25, 2022

rwaskiewicz left a comment

rwaskiewicz Apr 28, 2022

rwaskiewicz Apr 28, 2022

rwaskiewicz Apr 28, 2022

alicewriteswrongs commented May 10, 2022

	if (flags && flags.address && isString(flags.address)) {
	if (flags.address && isString(flags.address)) {

	export const isNumber = (v: any): v is boolean => typeof v === 'number';
	export const isNumber = (v: any): v is number => typeof v === 'number';

test(config): remove strictNullChecks errors from compiler/config #3335

test(config): remove strictNullChecks errors from compiler/config #3335

Conversation

alicewriteswrongs commented Apr 15, 2022 • edited Loading

Pull request checklist

Pull request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Testing

Other information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicewriteswrongs commented Apr 25, 2022

rwaskiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicewriteswrongs commented May 10, 2022

alicewriteswrongs commented Apr 15, 2022 •

edited

Loading