Skip to content

Commit 9b5b953

Browse files
test(config): remove strictNullChecks errors from compiler/config
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 (`?.`).
1 parent c9b4fad commit 9b5b953

15 files changed

+170
-114
lines changed

src/compiler/config/config-utils.ts

+41-51
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,40 @@
11
import type * as d from '../../declarations';
22
import { isAbsolute, join } from 'path';
3+
import { isBoolean } from '@utils';
34

4-
export const getAbsolutePath = (config: d.Config, dir: string) => {
5+
export const getAbsolutePath = (config: d.Config | d.UnvalidatedConfig, dir: string) => {
56
if (!isAbsolute(dir)) {
67
dir = join(config.rootDir, dir);
78
}
89
return dir;
910
};
1011

11-
export const setBooleanConfig = (config: any, configName: string, flagName: string, defaultValue: boolean) => {
12+
/**
13+
* This function does two things:
14+
*
15+
* 1. If you pass a `flagName`, it will hoist that `flagName` out of the
16+
* `ConfigFlags` object and onto the 'root' level (if you will) of the
17+
* `config` under the `configName` (`keyof d.Config`) that you pass.
18+
* 2. If you _don't_ pass a `flagName` it will just set the value you supply
19+
* on the config.
20+
*
21+
* @param config the config that we want to update
22+
* @param configName the key we're setting on the config
23+
* @param flagName either the name of a ConfigFlag prop we want to hoist up or null
24+
* @param defaultValue the default value we should set!
25+
*/
26+
export const setBooleanConfig = <K extends keyof d.Config>(
27+
config: d.UnvalidatedConfig,
28+
configName: K,
29+
flagName: keyof d.ConfigFlags | null,
30+
defaultValue: d.Config[K]
31+
) => {
1232
if (flagName) {
13-
if (typeof config.flags[flagName] === 'boolean') {
14-
config[configName] = config.flags[flagName];
33+
// I can't think of a great way to tell the compiler that `typeof Config[K]` is going
34+
// to be equal to `typeof ConfigFlags[K]`, so we lean on a little assertion 🫤
35+
let flagValue = config?.flags?.[flagName] as d.Config[K];
36+
if (isBoolean(flagValue)) {
37+
config[configName] = flagValue;
1538
}
1639
}
1740

@@ -21,64 +44,31 @@ export const setBooleanConfig = (config: any, configName: string, flagName: stri
2144
config[userConfigName] = !!config[userConfigName]();
2245
}
2346

24-
if (typeof config[userConfigName] === 'boolean') {
47+
if (isBoolean(config[userConfigName])) {
2548
config[configName] = config[userConfigName];
2649
} else {
2750
config[configName] = defaultValue;
2851
}
2952
};
3053

31-
export const setNumberConfig = (config: any, configName: string, _flagName: string, defaultValue: number) => {
32-
const userConfigName = getUserConfigName(config, configName);
33-
34-
if (typeof config[userConfigName] === 'function') {
35-
config[userConfigName] = config[userConfigName]();
36-
}
37-
38-
if (typeof config[userConfigName] === 'number') {
39-
config[configName] = config[userConfigName];
40-
} else {
41-
config[configName] = defaultValue;
42-
}
43-
};
44-
45-
export const setStringConfig = (config: any, configName: string, defaultValue: string) => {
46-
const userConfigName = getUserConfigName(config, configName);
47-
48-
if (typeof config[userConfigName] === 'function') {
49-
config[userConfigName] = config[userConfigName]();
50-
}
51-
52-
if (typeof config[userConfigName] === 'string') {
53-
config[configName] = config[userConfigName];
54-
} else {
55-
config[configName] = defaultValue;
56-
}
57-
};
58-
59-
export const setArrayConfig = (config: any, configName: string, defaultValue?: any[]) => {
60-
const userConfigName = getUserConfigName(config, configName);
61-
62-
if (typeof config[userConfigName] === 'function') {
63-
config[userConfigName] = config[userConfigName]();
64-
}
65-
66-
if (!Array.isArray(config[configName])) {
67-
if (Array.isArray(defaultValue)) {
68-
config[configName] = defaultValue.slice();
69-
} else {
70-
config[configName] = [];
71-
}
72-
}
73-
};
74-
75-
const getUserConfigName = (config: d.Config, correctConfigName: string) => {
54+
/**
55+
* Find any possibly mis-capitalized configuration names on the config, logging
56+
* a little warning for the user to let them know. This lets us recover values
57+
* set under (a subset of) improperly spelled configs and automatically hoist
58+
* them into the config under the right key.
59+
*
60+
* @param config d.Config
61+
* @param correctConfigName the configuration name that we're checking for right now
62+
* @returns a string container a mis-capitalized config name found on the
63+
* config object, if any.
64+
*/
65+
const getUserConfigName = (config: d.UnvalidatedConfig, correctConfigName: keyof d.Config): string => {
7666
const userConfigNames = Object.keys(config);
7767

7868
for (const userConfigName of userConfigNames) {
7969
if (userConfigName.toLowerCase() === correctConfigName.toLowerCase()) {
8070
if (userConfigName !== correctConfigName) {
81-
config.logger.warn(`config "${userConfigName}" should be "${correctConfigName}"`);
71+
config.logger?.warn(`config "${userConfigName}" should be "${correctConfigName}"`);
8272
return userConfigName;
8373
}
8474
break;

src/compiler/config/load-config.ts

+14-4
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,10 @@
1-
import type { CompilerSystem, Config, Diagnostic, LoadConfigInit, LoadConfigResults } from '../../declarations';
1+
import type {
2+
CompilerSystem,
3+
Diagnostic,
4+
LoadConfigInit,
5+
LoadConfigResults,
6+
UnvalidatedConfig,
7+
} from '../../declarations';
28
import { buildError, catchError, hasError, isString, normalizePath } from '@utils';
39
import { createLogger } from '../sys/logger/console-logger';
410
import { createSystem } from '../sys/stencil-sys';
@@ -89,8 +95,12 @@ export const loadConfig = async (init: LoadConfigInit = {}) => {
8995
return results;
9096
};
9197

92-
const loadConfigFile = async (sys: CompilerSystem, diagnostics: Diagnostic[], configPath: string) => {
93-
let config: Config = null;
98+
const loadConfigFile = async (
99+
sys: CompilerSystem,
100+
diagnostics: Diagnostic[],
101+
configPath: string
102+
): Promise<UnvalidatedConfig> => {
103+
let config: UnvalidatedConfig = null;
94104

95105
if (isString(configPath)) {
96106
// the passed in config was a string, so it's probably a path to the config we need to load
@@ -113,7 +123,7 @@ const loadConfigFile = async (sys: CompilerSystem, diagnostics: Diagnostic[], co
113123
};
114124

115125
const evaluateConfigFile = async (sys: CompilerSystem, diagnostics: Diagnostic[], configFilePath: string) => {
116-
let configFileData: { config?: Config } = null;
126+
let configFileData: { config?: UnvalidatedConfig } = null;
117127

118128
try {
119129
if (IS_NODE_ENV) {

src/compiler/config/outputs/index.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import { validateStats } from './validate-stats';
1313
import { validateWww } from './validate-www';
1414
import { validateCustomElementBundle } from './validate-custom-element-bundle';
1515

16-
export const validateOutputTargets = (config: d.Config, diagnostics: d.Diagnostic[]) => {
16+
export const validateOutputTargets = (config: d.UnvalidatedConfig, diagnostics: d.Diagnostic[]) => {
1717
const userOutputs = (config.outputTargets || []).slice();
1818

1919
userOutputs.forEach((outputTarget) => {

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ describe('validateDevServer', () => {
150150
it('should set address, port null, protocol', () => {
151151
inputConfig.devServer.address = 'https://subdomain.stenciljs.com/';
152152
const { config } = validateConfig(inputConfig);
153-
expect(config.devServer.port).toBe(null);
153+
expect(config.devServer.port).toBe(undefined);
154154
expect(config.devServer.address).toBe('subdomain.stenciljs.com');
155155
expect(config.devServer.protocol).toBe('https');
156156
});

src/compiler/config/validate-config.ts

+22-6
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { Config, ConfigBundle, Diagnostic } from '../../declarations';
1+
import { Config, ConfigBundle, Diagnostic, UnvalidatedConfig } from '../../declarations';
22
import { buildError, isBoolean, isNumber, isString, sortBy } from '@utils';
33
import { setBooleanConfig } from './config-utils';
44
import { validateDevServer } from './validate-dev-server';
@@ -12,17 +12,30 @@ import { validateRollupConfig } from './validate-rollup-config';
1212
import { validateTesting } from './validate-testing';
1313
import { validateWorkers } from './validate-workers';
1414

15-
export const validateConfig = (userConfig?: Config) => {
15+
/**
16+
* Validate a Config object, ensuring that all its field are present and
17+
* consistent with our expectations. This function transforms an
18+
* `UnvalidatedConfig` to a `Config`.
19+
*
20+
* @param userConfig an unvalidated config that we've gotten from a user
21+
* @returns an object with config and diagnostics props
22+
*/
23+
export const validateConfig = (
24+
userConfig: UnvalidatedConfig = {}
25+
): {
26+
config: Config;
27+
diagnostics: Diagnostic[];
28+
} => {
1629
const config = Object.assign({}, userConfig || {}); // not positive it's json safe
1730
const diagnostics: Diagnostic[] = [];
1831

1932
// copy flags (we know it'll be json safe)
2033
config.flags = JSON.parse(JSON.stringify(config.flags || {}));
2134

2235
// default devMode false
23-
if (config.flags.prod) {
36+
if (config?.flags?.prod) {
2437
config.devMode = false;
25-
} else if (config.flags.dev) {
38+
} else if (config?.flags?.dev) {
2639
config.devMode = true;
2740
} else if (!isBoolean(config.devMode)) {
2841
config.devMode = DEFAULT_DEV_MODE;
@@ -48,7 +61,7 @@ export const validateConfig = (userConfig?: Config) => {
4861
setBooleanConfig(config, 'sourceMap', null, typeof config.sourceMap === 'undefined' ? false : config.sourceMap);
4962
setBooleanConfig(config, 'watch', 'watch', false);
5063
setBooleanConfig(config, 'buildDocs', 'docs', !config.devMode);
51-
setBooleanConfig(config, 'buildDist', 'esm', !config.devMode || config.buildEs5);
64+
setBooleanConfig(config, 'buildDist', null, !config.devMode || config.buildEs5);
5265
setBooleanConfig(config, 'profile', 'profile', config.devMode);
5366
setBooleanConfig(config, 'writeLog', 'log', false);
5467
setBooleanConfig(config, 'buildAppCore', null, true);
@@ -132,8 +145,11 @@ export const validateConfig = (userConfig?: Config) => {
132145
return arr;
133146
}, [] as RegExp[]);
134147

148+
// this is well justified I promise :)
149+
let validatedConfig: Config = config as any;
150+
135151
return {
136-
config,
152+
config: validatedConfig,
137153
diagnostics,
138154
};
139155
};

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

+27-23
Original file line numberDiff line numberDiff line change
@@ -3,21 +3,25 @@ import { buildError, isBoolean, isNumber, isString, normalizePath } from '@utils
33
import { isAbsolute, join } from 'path';
44
import { isOutputTargetWww } from '../output-targets/output-utils';
55

6-
export const validateDevServer = (config: d.Config, diagnostics: d.Diagnostic[]) => {
6+
export const validateDevServer = (
7+
config: d.UnvalidatedConfig,
8+
diagnostics: d.Diagnostic[]
9+
): d.DevServerConfig | undefined => {
710
if ((config.devServer === null || (config.devServer as any)) === false) {
8-
return null;
11+
return undefined;
912
}
1013

1114
const flags = config.flags;
1215
const devServer = { ...config.devServer };
1316

14-
if (isString(flags.address)) {
17+
if (flags && flags.address && isString(flags.address)) {
1518
devServer.address = flags.address;
1619
} else if (!isString(devServer.address)) {
1720
devServer.address = '0.0.0.0';
1821
}
1922

20-
let addressProtocol: 'http' | 'https';
23+
// default to http for localdev
24+
let addressProtocol: 'http' | 'https' = 'http';
2125
if (devServer.address.toLowerCase().startsWith('http://')) {
2226
devServer.address = devServer.address.substring(7);
2327
addressProtocol = 'http';
@@ -28,24 +32,23 @@ export const validateDevServer = (config: d.Config, diagnostics: d.Diagnostic[])
2832

2933
devServer.address = devServer.address.split('/')[0];
3034

31-
let addressPort: number;
3235
const addressSplit = devServer.address.split(':');
3336
if (addressSplit.length > 1) {
3437
if (!isNaN(addressSplit[1] as any)) {
3538
devServer.address = addressSplit[0];
36-
addressPort = parseInt(addressSplit[1], 10);
39+
devServer.port = parseInt(addressSplit[1], 10);
3740
}
3841
}
3942

40-
if (isNumber(flags.port)) {
41-
devServer.port = flags.port;
42-
} else if (devServer.port !== null && !isNumber(devServer.port)) {
43-
if (isNumber(addressPort)) {
44-
devServer.port = addressPort;
45-
} else if (devServer.address === 'localhost' || !isNaN(devServer.address.split('.')[0] as any)) {
43+
if (isNumber(flags?.port)) {
44+
devServer.port = flags?.port;
45+
}
46+
47+
if (devServer.port !== null && !isNumber(devServer.port)) {
48+
if (devServer.address === 'localhost' || !isNaN(devServer.address.split('.')[0] as any)) {
4649
devServer.port = 3333;
4750
} else {
48-
devServer.port = null;
51+
devServer.port = undefined;
4952
}
5053
}
5154

@@ -79,7 +82,7 @@ export const validateDevServer = (config: d.Config, diagnostics: d.Diagnostic[])
7982
}
8083

8184
if (devServer.ssr) {
82-
const wwwOutput = config.outputTargets.find(isOutputTargetWww);
85+
const wwwOutput = (config.outputTargets ?? []).find(isOutputTargetWww);
8386
devServer.prerenderConfig = wwwOutput?.prerenderConfig;
8487
}
8588

@@ -103,22 +106,23 @@ export const validateDevServer = (config: d.Config, diagnostics: d.Diagnostic[])
103106
}
104107
}
105108

106-
if (flags.open === false) {
109+
if (flags?.open === false) {
107110
devServer.openBrowser = false;
108-
} else if (flags.prerender && !config.watch) {
111+
} else if (flags?.prerender && !config.watch) {
109112
devServer.openBrowser = false;
110113
}
111114

112-
let serveDir: string = null;
113-
let basePath: string = null;
114-
const wwwOutputTarget = config.outputTargets.find(isOutputTargetWww);
115+
let serveDir: string;
116+
let basePath: string;
117+
const wwwOutputTarget = (config.outputTargets ?? []).find(isOutputTargetWww);
115118

116119
if (wwwOutputTarget) {
117-
const baseUrl = new URL(wwwOutputTarget.baseUrl, 'http://config.stenciljs.com');
120+
const baseUrl = new URL(wwwOutputTarget.baseUrl ?? '', 'http://config.stenciljs.com');
118121
basePath = baseUrl.pathname;
119-
serveDir = wwwOutputTarget.appDir;
122+
serveDir = wwwOutputTarget.appDir ?? '';
120123
} else {
121-
serveDir = config.rootDir;
124+
basePath = '';
125+
serveDir = config.rootDir ?? '';
122126
}
123127

124128
if (!isString(basePath) || basePath.trim() === '') {
@@ -153,7 +157,7 @@ export const validateDevServer = (config: d.Config, diagnostics: d.Diagnostic[])
153157
}
154158

155159
if (!isAbsolute(devServer.root)) {
156-
devServer.root = join(config.rootDir, devServer.root);
160+
devServer.root = join(config.rootDir as string, devServer.root);
157161
}
158162
devServer.root = normalizePath(devServer.root);
159163

src/compiler/config/validate-hydrated.ts

+4-4
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
1-
import { Config, HydratedFlag } from '../../declarations';
1+
import { HydratedFlag, UnvalidatedConfig } from '../../declarations';
22
import { isString } from '@utils';
33

4-
export const validateHydrated = (config: Config) => {
5-
if (config.hydratedFlag === null || config.hydratedFlag === false) {
6-
return null;
4+
export const validateHydrated = (config: UnvalidatedConfig) => {
5+
if (config.hydratedFlag === undefined || config.hydratedFlag === null || config.hydratedFlag === false) {
6+
return undefined;
77
}
88

99
const hydratedFlag: HydratedFlag = { ...config.hydratedFlag };

src/compiler/config/validate-namespace.ts

+3-3
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import type * as d from '../../declarations';
22
import { buildError, dashToPascalCase, isString } from '@utils';
33
import { isOutputTargetDist } from '../output-targets/output-utils';
44

5-
export const validateNamespace = (c: d.Config, diagnostics: d.Diagnostic[]) => {
5+
export const validateNamespace = (c: d.UnvalidatedConfig, diagnostics: d.Diagnostic[]) => {
66
c.namespace = isString(c.namespace) ? c.namespace : DEFAULT_NAMESPACE;
77
c.namespace = c.namespace.trim();
88

@@ -40,8 +40,8 @@ export const validateNamespace = (c: d.Config, diagnostics: d.Diagnostic[]) => {
4040
}
4141
};
4242

43-
export const validateDistNamespace = (config: d.Config, diagnostics: d.Diagnostic[]) => {
44-
const hasDist = config.outputTargets.some(isOutputTargetDist);
43+
export const validateDistNamespace = (config: d.UnvalidatedConfig, diagnostics: d.Diagnostic[]) => {
44+
const hasDist = (config.outputTargets ?? []).some(isOutputTargetDist);
4545
if (hasDist) {
4646
if (!isString(config.namespace) || config.namespace.toLowerCase() === 'app') {
4747
const err = buildError(diagnostics);

src/compiler/config/validate-paths.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import type * as d from '../../declarations';
22
import { isAbsolute, join } from 'path';
33

4-
export const validatePaths = (config: d.Config) => {
4+
export const validatePaths = (config: d.UnvalidatedConfig) => {
55
if (typeof config.rootDir !== 'string') {
66
config.rootDir = '/';
77
}

0 commit comments

Comments
 (0)