Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 12 commits into from
May 12, 2022
Merged

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

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
57f87ff
test(config): remove strictNullChecks errors from compiler/config
alicewriteswrongs Apr 14, 2022
4f64b8a
remove the `as` cast
alicewriteswrongs Apr 25, 2022
f748d28
fix some typings on those `is${TypeName}` functions
alicewriteswrongs Apr 25, 2022
d42d9b7
declare a flags if none
alicewriteswrongs Apr 25, 2022
8b8b314
responding to feedback
alicewriteswrongs Apr 27, 2022
ab9f680
addressing some more feedback
alicewriteswrongs Apr 27, 2022
fd2dbbc
more feedback
alicewriteswrongs Apr 27, 2022
2a9ec5b
some feedback changes
alicewriteswrongs Apr 29, 2022
85acf7b
temporarily put back the logic from main in validate-dev-server.ts
alicewriteswrongs Apr 29, 2022
9c536ed
update logic for port, address, etc in validate-dev-server
alicewriteswrongs Apr 29, 2022
fba2ade
fix issue causing karma tests to fail
alicewriteswrongs May 10, 2022
c724aff
Merge branch 'main' into ap/config-null-checks
rwaskiewicz May 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 37 additions & 51 deletions src/compiler/config/config-utils.ts
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why we changed this key name here. Should this be 'esm' still?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure about that - this argument is supposed to be of type keyof d.ConfigFlags and esm is not a property on ConfigFlags so, assuming that ConfigFlags objects actually conform to that interface there shouldn't ever be a value under the "esm" key to pull out - I changed it for that reason, but it could be the case that we should keep it any, if we wanted to I think we'd need to add an esm: boolean prop to ConfigFlags

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
View file
Open in desktop
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