Skip to content

Commit

Permalink
test(config): remove strictNullChecks errors from compiler/config
Browse files Browse the repository at this point in the history
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 (`?.`).
  • Loading branch information
alicewriteswrongs committed Apr 22, 2022
1 parent 31eae6e commit 9266dda
Show file tree
Hide file tree
Showing 15 changed files with 170 additions and 114 deletions.
92 changes: 41 additions & 51 deletions src/compiler/config/config-utils.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,40 @@
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,
flagName: keyof d.ConfigFlags | null,
defaultValue: d.Config[K]
) => {
if (flagName) {
if (typeof config.flags[flagName] === 'boolean') {
config[configName] = config.flags[flagName];
// I can't think of a great way to tell the compiler that `typeof Config[K]` is going
// to be equal to `typeof ConfigFlags[K]`, so we lean on a little assertion 🫤
let flagValue = config?.flags?.[flagName] as d.Config[K];
if (isBoolean(flagValue)) {
config[configName] = flagValue;
}
}

Expand All @@ -21,64 +44,31 @@ 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
* a little warning for the user to let them know. This lets us recover values
* set under (a subset of) improperly spelled configs and automatically hoist
* them into the config under the right key.
*
* @param config d.Config
* @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
2 changes: 1 addition & 1 deletion src/compiler/config/test/validate-dev-server.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ describe('validateDevServer', () => {
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
28 changes: 22 additions & 6 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,17 +12,30 @@ 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[] = [];

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

// default devMode false
if (config.flags.prod) {
if (config?.flags?.prod) {
config.devMode = false;
} else if (config.flags.dev) {
} else if (config?.flags?.dev) {
config.devMode = true;
} else if (!isBoolean(config.devMode)) {
config.devMode = DEFAULT_DEV_MODE;
Expand All @@ -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 Expand Up @@ -132,8 +145,11 @@ export const validateConfig = (userConfig?: Config) => {
return arr;
}, [] as RegExp[]);

// this is well justified I promise :)
let validatedConfig: Config = config as any;

return {
config,
config: validatedConfig,
diagnostics,
};
};
Expand Down
50 changes: 27 additions & 23 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 devServer = { ...config.devServer };

if (isString(flags.address)) {
if (flags && 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,24 +32,23 @@ export const validateDevServer = (config: d.Config, diagnostics: d.Diagnostic[])

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

let addressPort: number;
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);
}
}

if (isNumber(flags.port)) {
devServer.port = flags.port;
} 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)) {
if (isNumber(flags?.port)) {
devServer.port = flags?.port;
}

if (devServer.port !== null && !isNumber(devServer.port)) {
if (devServer.address === 'localhost' || !isNaN(devServer.address.split('.')[0] as any)) {
devServer.port = 3333;
} else {
devServer.port = null;
devServer.port = undefined;
}
}

Expand Down Expand Up @@ -79,7 +82,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 All @@ -103,22 +106,23 @@ export const validateDevServer = (config: d.Config, diagnostics: d.Diagnostic[])
}
}

if (flags.open === false) {
if (flags?.open === false) {
devServer.openBrowser = false;
} else if (flags.prerender && !config.watch) {
} else if (flags?.prerender && !config.watch) {
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 +157,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
8 changes: 4 additions & 4 deletions src/compiler/config/validate-hydrated.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { Config, HydratedFlag } from '../../declarations';
import { HydratedFlag, UnvalidatedConfig } from '../../declarations';
import { isString } from '@utils';

export const validateHydrated = (config: Config) => {
if (config.hydratedFlag === null || config.hydratedFlag === false) {
return null;
export const validateHydrated = (config: UnvalidatedConfig) => {
if (config.hydratedFlag === undefined || config.hydratedFlag === null || config.hydratedFlag === false) {
return undefined;
}

const hydratedFlag: HydratedFlag = { ...config.hydratedFlag };
Expand Down
6 changes: 3 additions & 3 deletions src/compiler/config/validate-namespace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import type * as d from '../../declarations';
import { buildError, dashToPascalCase, isString } from '@utils';
import { isOutputTargetDist } from '../output-targets/output-utils';

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

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

export const validateDistNamespace = (config: d.Config, diagnostics: d.Diagnostic[]) => {
const hasDist = config.outputTargets.some(isOutputTargetDist);
export const validateDistNamespace = (config: d.UnvalidatedConfig, diagnostics: d.Diagnostic[]) => {
const hasDist = (config.outputTargets ?? []).some(isOutputTargetDist);
if (hasDist) {
if (!isString(config.namespace) || config.namespace.toLowerCase() === 'app') {
const err = buildError(diagnostics);
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/config/validate-paths.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type * as d from '../../declarations';
import { isAbsolute, join } from 'path';

export const validatePaths = (config: d.Config) => {
export const validatePaths = (config: d.UnvalidatedConfig) => {
if (typeof config.rootDir !== 'string') {
config.rootDir = '/';
}
Expand Down
Loading

0 comments on commit 9266dda

Please sign in to comment.