Skip to content

Commit

Permalink
refactor(config): remove strictNullChecks errors from compiler/config (
Browse files Browse the repository at this point in the history
…#3335)

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 code base 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 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 (`?.`).
  • Loading branch information
alicewriteswrongs authored May 12, 2022
1 parent 0cd139b commit cca8951
Show file tree
Hide file tree
Showing 15 changed files with 201 additions and 147 deletions.
88 changes: 37 additions & 51 deletions src/compiler/config/config-utils.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,38 @@
import type * as d from '../../declarations';
import { isAbsolute, join } from 'path';
import { isBoolean } from '@utils';

export const getAbsolutePath = (config: d.Config, dir: string) => {
export const getAbsolutePath = (config: d.Config | d.UnvalidatedConfig, dir: string) => {
if (!isAbsolute(dir)) {
dir = join(config.rootDir, dir);
}
return dir;
};

export const setBooleanConfig = (config: any, configName: string, flagName: string, defaultValue: boolean) => {
/**
* This function does two things:
*
* 1. If you pass a `flagName`, it will hoist that `flagName` out of the
* `ConfigFlags` object and onto the 'root' level (if you will) of the
* `config` under the `configName` (`keyof d.Config`) that you pass.
* 2. If you _don't_ pass a `flagName` it will just set the value you supply
* on the config.
*
* @param config the config that we want to update
* @param configName the key we're setting on the config
* @param flagName either the name of a ConfigFlag prop we want to hoist up or null
* @param defaultValue the default value we should set!
*/
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) {
if (typeof config.flags[flagName] === 'boolean') {
config[configName] = config.flags[flagName];
let flagValue = config.flags?.[flagName];
if (isBoolean(flagValue)) {
config[configName] = flagValue;
}
}

Expand All @@ -21,64 +42,29 @@ export const setBooleanConfig = (config: any, configName: string, flagName: stri
config[userConfigName] = !!config[userConfigName]();
}

if (typeof config[userConfigName] === 'boolean') {
if (isBoolean(config[userConfigName])) {
config[configName] = config[userConfigName];
} else {
config[configName] = defaultValue;
}
};

export const setNumberConfig = (config: any, configName: string, _flagName: string, defaultValue: number) => {
const userConfigName = getUserConfigName(config, configName);

if (typeof config[userConfigName] === 'function') {
config[userConfigName] = config[userConfigName]();
}

if (typeof config[userConfigName] === 'number') {
config[configName] = config[userConfigName];
} else {
config[configName] = defaultValue;
}
};

export const setStringConfig = (config: any, configName: string, defaultValue: string) => {
const userConfigName = getUserConfigName(config, configName);

if (typeof config[userConfigName] === 'function') {
config[userConfigName] = config[userConfigName]();
}

if (typeof config[userConfigName] === 'string') {
config[configName] = config[userConfigName];
} else {
config[configName] = defaultValue;
}
};

export const setArrayConfig = (config: any, configName: string, defaultValue?: any[]) => {
const userConfigName = getUserConfigName(config, configName);

if (typeof config[userConfigName] === 'function') {
config[userConfigName] = config[userConfigName]();
}

if (!Array.isArray(config[configName])) {
if (Array.isArray(defaultValue)) {
config[configName] = defaultValue.slice();
} else {
config[configName] = [];
}
}
};

const getUserConfigName = (config: d.Config, correctConfigName: string) => {
/**
* Find any possibly mis-capitalized configuration names on the config, logging
* and warning if one is found.
*
* @param config the user-supplied config that we're dealing with
* @param correctConfigName the configuration name that we're checking for right now
* @returns a string container a mis-capitalized config name found on the
* config object, if any.
*/
const getUserConfigName = (config: d.UnvalidatedConfig, correctConfigName: keyof d.Config): string => {
const userConfigNames = Object.keys(config);

for (const userConfigName of userConfigNames) {
if (userConfigName.toLowerCase() === correctConfigName.toLowerCase()) {
if (userConfigName !== correctConfigName) {
config.logger.warn(`config "${userConfigName}" should be "${correctConfigName}"`);
config.logger?.warn(`config "${userConfigName}" should be "${correctConfigName}"`);
return userConfigName;
}
break;
Expand Down
18 changes: 14 additions & 4 deletions src/compiler/config/load-config.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
import type { CompilerSystem, Config, Diagnostic, LoadConfigInit, LoadConfigResults } from '../../declarations';
import type {
CompilerSystem,
Diagnostic,
LoadConfigInit,
LoadConfigResults,
UnvalidatedConfig,
} from '../../declarations';
import { buildError, catchError, hasError, isString, normalizePath } from '@utils';
import { createLogger } from '../sys/logger/console-logger';
import { createSystem } from '../sys/stencil-sys';
Expand Down Expand Up @@ -89,8 +95,12 @@ export const loadConfig = async (init: LoadConfigInit = {}) => {
return results;
};

const loadConfigFile = async (sys: CompilerSystem, diagnostics: Diagnostic[], configPath: string) => {
let config: Config = null;
const loadConfigFile = async (
sys: CompilerSystem,
diagnostics: Diagnostic[],
configPath: string
): Promise<UnvalidatedConfig> => {
let config: UnvalidatedConfig = null;

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

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

try {
if (IS_NODE_ENV) {
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/config/outputs/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { validateStats } from './validate-stats';
import { validateWww } from './validate-www';
import { validateCustomElementBundle } from './validate-custom-element-bundle';

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

userOutputs.forEach((outputTarget) => {
Expand Down
63 changes: 22 additions & 41 deletions src/compiler/config/test/validate-dev-server.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,23 +24,14 @@ describe('validateDevServer', () => {
expect(config.devServer.address).toBe('0.0.0.0');
});

it('should remove http from address', () => {
inputConfig.devServer.address = 'http://localhost';
const { config } = validateConfig(inputConfig);
expect(config.devServer.address).toBe('localhost');
});

it('should remove https from address', () => {
inputConfig.devServer.address = 'https://localhost';
const { config } = validateConfig(inputConfig);
expect(config.devServer.address).toBe('localhost');
});

it('should remove trailing / from address', () => {
inputConfig.devServer.address = 'localhost/';
const { config } = validateConfig(inputConfig);
expect(config.devServer.address).toBe('localhost');
});
it.each(['https://localhost', 'http://localhost', 'https://localhost/', 'http://localhost/', 'localhost/'])(
'should remove extraneious stuff from addres %p',
(address) => {
inputConfig.devServer.address = address;
const { config } = validateConfig(inputConfig);
expect(config.devServer.address).toBe('localhost');
}
);

it('should set address', () => {
inputConfig.devServer.address = '123.123.123.123';
Expand Down Expand Up @@ -109,48 +100,38 @@ describe('validateDevServer', () => {
expect(config.devServer.gzip).toBe(false);
});

it('should default port', () => {
const { config } = validateConfig(inputConfig);
expect(config.devServer.port).toBe(3333);
});

it('should default port with ip address', () => {
inputConfig.devServer.address = '192.12.12.10';
it.each(['localhost', '192.12.12.10', '127.0.0.1'])('should default port with address %p', () => {
const { config } = validateConfig(inputConfig);
expect(config.devServer.port).toBe(3333);
});

it('should default port with localhost', () => {
inputConfig.devServer.address = 'localhost';
const { config } = validateConfig(inputConfig);
expect(config.devServer.port).toBe(3333);
});
it.each(['https://subdomain.stenciljs.com:3000', 'localhost:3000/', 'localhost:3000'])(
'should override port in address with port property',
(address) => {
inputConfig.devServer.port = 1234;
inputConfig.devServer.address = address;
const { config } = validateConfig(inputConfig);
expect(config.devServer.port).toBe(1234);
}
);

it('should not set default port if null', () => {
inputConfig.devServer.port = null;
const { config } = validateConfig(inputConfig);
expect(config.devServer.port).toBe(null);
});

it('should set port if in address', () => {
inputConfig.devServer.address = 'localhost:88';
it.each(['localhost:20/', 'localhost:20'])('should set port from address %p if no port prop', (address) => {
inputConfig.devServer.address = address;
const { config } = validateConfig(inputConfig);
expect(config.devServer.port).toBe(88);
expect(config.devServer.port).toBe(20);
expect(config.devServer.address).toBe('localhost');
});

it('should set port if in address and has trailing slash', () => {
inputConfig.devServer.address = 'https://localhost:88/';
const { config } = validateConfig(inputConfig);
expect(config.devServer.port).toBe(88);
expect(config.devServer.address).toBe('localhost');
expect(config.devServer.protocol).toBe('https');
});

it('should set address, port null, protocol', () => {
inputConfig.devServer.address = 'https://subdomain.stenciljs.com/';
const { config } = validateConfig(inputConfig);
expect(config.devServer.port).toBe(null);
expect(config.devServer.port).toBe(undefined);
expect(config.devServer.address).toBe('subdomain.stenciljs.com');
expect(config.devServer.protocol).toBe('https');
});
Expand Down
19 changes: 16 additions & 3 deletions src/compiler/config/validate-config.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Config, ConfigBundle, Diagnostic } from '../../declarations';
import { Config, ConfigBundle, Diagnostic, UnvalidatedConfig } from '../../declarations';
import { buildError, isBoolean, isNumber, isString, sortBy } from '@utils';
import { setBooleanConfig } from './config-utils';
import { validateDevServer } from './validate-dev-server';
Expand All @@ -12,7 +12,20 @@ import { validateRollupConfig } from './validate-rollup-config';
import { validateTesting } from './validate-testing';
import { validateWorkers } from './validate-workers';

export const validateConfig = (userConfig?: Config) => {
/**
* Validate a Config object, ensuring that all its field are present and
* consistent with our expectations. This function transforms an
* `UnvalidatedConfig` to a `Config`.
*
* @param userConfig an unvalidated config that we've gotten from a user
* @returns an object with config and diagnostics props
*/
export const validateConfig = (
userConfig: UnvalidatedConfig = {}
): {
config: Config;
diagnostics: Diagnostic[];
} => {
const config = Object.assign({}, userConfig || {}); // not positive it's json safe
const diagnostics: Diagnostic[] = [];

Expand Down Expand Up @@ -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);
setBooleanConfig(config, 'profile', 'profile', config.devMode);
setBooleanConfig(config, 'writeLog', 'log', false);
setBooleanConfig(config, 'buildAppCore', null, true);
Expand Down
45 changes: 27 additions & 18 deletions src/compiler/config/validate-dev-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,21 +3,25 @@ import { buildError, isBoolean, isNumber, isString, normalizePath } from '@utils
import { isAbsolute, join } from 'path';
import { isOutputTargetWww } from '../output-targets/output-utils';

export const validateDevServer = (config: d.Config, diagnostics: d.Diagnostic[]) => {
export const validateDevServer = (
config: d.UnvalidatedConfig,
diagnostics: d.Diagnostic[]
): d.DevServerConfig | undefined => {
if ((config.devServer === null || (config.devServer as any)) === false) {
return null;
return undefined;
}

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

if (isString(flags.address)) {
if (flags.address && isString(flags.address)) {
devServer.address = flags.address;
} else if (!isString(devServer.address)) {
devServer.address = '0.0.0.0';
}

let addressProtocol: 'http' | 'https';
// default to http for localdev
let addressProtocol: 'http' | 'https' = 'http';
if (devServer.address.toLowerCase().startsWith('http://')) {
devServer.address = devServer.address.substring(7);
addressProtocol = 'http';
Expand All @@ -28,8 +32,16 @@ export const validateDevServer = (config: d.Config, diagnostics: d.Diagnostic[])

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

let addressPort: number;
// split on `:` to get the domain and the (possibly present) port
// separately. we've already sliced off the protocol (if present) above
// so we can safely split on `:` here.
const addressSplit = devServer.address.split(':');

const isLocalhost = addressSplit[0] === 'localhost' || !isNaN(addressSplit[0].split('.')[0] as any);

// if localhost we use 3333 as a default port
let addressPort: number | undefined = isLocalhost ? 3333 : undefined;

if (addressSplit.length > 1) {
if (!isNaN(addressSplit[1] as any)) {
devServer.address = addressSplit[0];
Expand All @@ -42,10 +54,6 @@ export const validateDevServer = (config: d.Config, diagnostics: d.Diagnostic[])
} else if (devServer.port !== null && !isNumber(devServer.port)) {
if (isNumber(addressPort)) {
devServer.port = addressPort;
} else if (devServer.address === 'localhost' || !isNaN(devServer.address.split('.')[0] as any)) {
devServer.port = 3333;
} else {
devServer.port = null;
}
}

Expand Down Expand Up @@ -79,7 +87,7 @@ export const validateDevServer = (config: d.Config, diagnostics: d.Diagnostic[])
}

if (devServer.ssr) {
const wwwOutput = config.outputTargets.find(isOutputTargetWww);
const wwwOutput = (config.outputTargets ?? []).find(isOutputTargetWww);
devServer.prerenderConfig = wwwOutput?.prerenderConfig;
}

Expand Down Expand Up @@ -109,16 +117,17 @@ export const validateDevServer = (config: d.Config, diagnostics: d.Diagnostic[])
devServer.openBrowser = false;
}

let serveDir: string = null;
let basePath: string = null;
const wwwOutputTarget = config.outputTargets.find(isOutputTargetWww);
let serveDir: string;
let basePath: string;
const wwwOutputTarget = (config.outputTargets ?? []).find(isOutputTargetWww);

if (wwwOutputTarget) {
const baseUrl = new URL(wwwOutputTarget.baseUrl, 'http://config.stenciljs.com');
const baseUrl = new URL(wwwOutputTarget.baseUrl ?? '', 'http://config.stenciljs.com');
basePath = baseUrl.pathname;
serveDir = wwwOutputTarget.appDir;
serveDir = wwwOutputTarget.appDir ?? '';
} else {
serveDir = config.rootDir;
basePath = '';
serveDir = config.rootDir ?? '';
}

if (!isString(basePath) || basePath.trim() === '') {
Expand Down Expand Up @@ -153,7 +162,7 @@ export const validateDevServer = (config: d.Config, diagnostics: d.Diagnostic[])
}

if (!isAbsolute(devServer.root)) {
devServer.root = join(config.rootDir, devServer.root);
devServer.root = join(config.rootDir as string, devServer.root);
}
devServer.root = normalizePath(devServer.root);

Expand Down
Loading

0 comments on commit cca8951

Please sign in to comment.