Skip to content

Commit

Permalink
feat(@angular-devkit/build-angular): support JSON comments in dev-ser…
Browse files Browse the repository at this point in the history 
…ver proxy configuration file

The `proxyConfig` option for the `dev-server` builder now supports JSON files containing comments and trailing commas.
Several additional tests regarding the use of ESM and CommonJS JavaScript configuration files were also added to reduce the likelihood of future regressions.

Closes: #21862
  • Loading branch information
@clydin @filipesilva
clydin authored and filipesilva committed Nov 16, 2021
1 parent 7d642fe commit 6d0f99a
Show file tree
Hide file tree
Showing 4 changed files with 176 additions and 15 deletions.
1 change: 1 addition & 0 deletions packages/angular_devkit/build_angular/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ ts_library(
"@npm//glob",
"@npm//https-proxy-agent",
"@npm//inquirer",
"@npm//jsonc-parser",
"@npm//karma",
"@npm//karma-source-map-support",
"@npm//less",
Expand Down
1 change: 1 addition & 0 deletions packages/angular_devkit/build_angular/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
"glob": "7.2.0",
"https-proxy-agent": "5.0.0",
"inquirer": "8.2.0",
"jsonc-parser": "3.0.0",
"karma-source-map-support": "1.4.0",
"less": "4.1.2",
"less-loader": "10.2.0",
Expand Down
106 changes: 103 additions & 3 deletions ...s/angular_devkit/build_angular/src/builders/dev-server/tests/options/proxy-config_spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ describeBuilder(serveWebpackBrowser, DEV_SERVER_BUILDER_INFO, (harness) => {
await harness.writeFile('src/main.ts', '');
});

it('proxies requests based on the JSON proxy configuration file provided in the option', async () => {
it('proxies requests based on the `.json` proxy configuration file provided in the option', async () => {
harness.useTarget('serve', {
...BASE_OPTIONS,
proxyConfig: 'proxy.config.json',
Expand All @@ -49,7 +49,34 @@ describeBuilder(serveWebpackBrowser, DEV_SERVER_BUILDER_INFO, (harness) => {
}
});

it('proxies requests based on the JS proxy configuration file provided in the option', async () => {
it('proxies requests based on the `.json` (with comments) proxy configuration file provided in the option', async () => {
harness.useTarget('serve', {
...BASE_OPTIONS,
proxyConfig: 'proxy.config.json',
});

const proxyServer = createProxyServer();
try {
await new Promise<void>((resolve) => proxyServer.listen(0, '127.0.0.1', resolve));
const proxyAddress = proxyServer.address() as import('net').AddressInfo;

await harness.writeFiles({
'proxy.config.json': `
// JSON file with comments
{ "/api/*": { "target": "http://127.0.0.1:${proxyAddress.port}" } }
`,
});

const { result, response } = await executeOnceAndFetch(harness, '/api/test');

expect(result?.success).toBeTrue();
expect(await response?.text()).toContain('TEST_API_RETURN');
} finally {
await new Promise<void>((resolve) => proxyServer.close(() => resolve()));
}
});

it('proxies requests based on the `.js` (CommonJS) proxy configuration file provided in the option', async () => {
harness.useTarget('serve', {
...BASE_OPTIONS,
proxyConfig: 'proxy.config.js',
Expand All @@ -73,7 +100,56 @@ describeBuilder(serveWebpackBrowser, DEV_SERVER_BUILDER_INFO, (harness) => {
}
});

it('proxies requests based on the MJS proxy configuration file provided in the option', async () => {
it('proxies requests based on the `.js` (ESM) proxy configuration file provided in the option', async () => {
harness.useTarget('serve', {
...BASE_OPTIONS,
proxyConfig: 'proxy.config.js',
});

const proxyServer = createProxyServer();
try {
await new Promise<void>((resolve) => proxyServer.listen(0, '127.0.0.1', resolve));
const proxyAddress = proxyServer.address() as import('net').AddressInfo;

await harness.writeFiles({
'proxy.config.js': `export default { "/api/*": { "target": "http://127.0.0.1:${proxyAddress.port}" } }`,
'package.json': '{ "type": "module" }',
});

const { result, response } = await executeOnceAndFetch(harness, '/api/test');

expect(result?.success).toBeTrue();
expect(await response?.text()).toContain('TEST_API_RETURN');
} finally {
await new Promise<void>((resolve) => proxyServer.close(() => resolve()));
}
});

it('proxies requests based on the `.cjs` proxy configuration file provided in the option', async () => {
harness.useTarget('serve', {
...BASE_OPTIONS,
proxyConfig: 'proxy.config.cjs',
});

const proxyServer = createProxyServer();
try {
await new Promise<void>((resolve) => proxyServer.listen(0, '127.0.0.1', resolve));
const proxyAddress = proxyServer.address() as import('net').AddressInfo;

await harness.writeFiles({
'proxy.config.cjs': `module.exports = { "/api/*": { "target": "http://127.0.0.1:${proxyAddress.port}" } }`,
});

const { result, response } = await executeOnceAndFetch(harness, '/api/test');

expect(result?.success).toBeTrue();
expect(await response?.text()).toContain('TEST_API_RETURN');
} finally {
await new Promise<void>((resolve) => proxyServer.close(() => resolve()));
}
});

it('proxies requests based on the `.mjs` proxy configuration file provided in the option', async () => {
harness.useTarget('serve', {
...BASE_OPTIONS,
proxyConfig: 'proxy.config.mjs',
Expand Down Expand Up @@ -112,6 +188,30 @@ describeBuilder(serveWebpackBrowser, DEV_SERVER_BUILDER_INFO, (harness) => {
}),
);
});

it('throws an error when JSON proxy configuration file cannot be parsed', async () => {
harness.useTarget('serve', {
...BASE_OPTIONS,
proxyConfig: 'proxy.config.json',
});

// Create a JSON file with a parse error (target property has no value)
await harness.writeFiles({
'proxy.config.json': `
// JSON file with comments
{ "/api/*": { "target": } }
`,
});

const { result, error } = await harness.executeOnce({ outputLogsOnException: false });

expect(result).toBeUndefined();
expect(error).toEqual(
jasmine.objectContaining({
message: jasmine.stringMatching('contains parse errors:\\n\\[3, 35\\] ValueExpected'),
}),
);
});
});
});

Expand Down
83 changes: 71 additions & 12 deletions packages/angular_devkit/build_angular/src/webpack/configs/dev-server.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
*/

import { logging, tags } from '@angular-devkit/core';
import { existsSync } from 'fs';
import { posix, resolve } from 'path';
import { existsSync, promises as fsPromises } from 'fs';
import { extname, posix, resolve } from 'path';
import * as url from 'url';
import { Configuration, RuleSetRule } from 'webpack';
import { Configuration as DevServerConfiguration } from 'webpack-dev-server';
Expand Down Expand Up @@ -164,22 +164,81 @@ async function addProxyConfig(root: string, proxyConfig: string | undefined) {
}

const proxyPath = resolve(root, proxyConfig);
if (existsSync(proxyPath)) {
try {

if (!existsSync(proxyPath)) {
throw new Error(`Proxy configuration file ${proxyPath} does not exist.`);
}

switch (extname(proxyPath)) {
case '.json': {
const content = await fsPromises.readFile(proxyPath, 'utf-8');

const { parse, printParseErrorCode } = await import('jsonc-parser');
const parseErrors: import('jsonc-parser').ParseError[] = [];
const proxyConfiguration = parse(content, parseErrors, { allowTrailingComma: true });

if (parseErrors.length > 0) {
let errorMessage = `Proxy configuration file ${proxyPath} contains parse errors:`;
for (const parseError of parseErrors) {
const { line, column } = getJsonErrorLineColumn(parseError.offset, content);
errorMessage += `\n[${line}, ${column}] ${printParseErrorCode(parseError.error)}`;
}
throw new Error(errorMessage);
}

return proxyConfiguration;
}
case '.mjs':
// Load the ESM configuration file using the TypeScript dynamic import workaround.
// Once TypeScript provides support for keeping the dynamic import this workaround can be
// changed to a direct dynamic import.
return (await loadEsmModule<{ default: unknown }>(url.pathToFileURL(proxyPath))).default;
case '.cjs':
return require(proxyPath);
} catch (e) {
if (e.code === 'ERR_REQUIRE_ESM') {
// Load the ESM configuration file using the TypeScript dynamic import workaround.
// Once TypeScript provides support for keeping the dynamic import this workaround can be
// changed to a direct dynamic import.
return (await loadEsmModule<{ default: unknown }>(url.pathToFileURL(proxyPath))).default;
default:
// The file could be either CommonJS or ESM.
// CommonJS is tried first then ESM if loading fails.
try {
return require(proxyPath);
} catch (e) {
if (e.code === 'ERR_REQUIRE_ESM') {
// Load the ESM configuration file using the TypeScript dynamic import workaround.
// Once TypeScript provides support for keeping the dynamic import this workaround can be
// changed to a direct dynamic import.
return (await loadEsmModule<{ default: unknown }>(url.pathToFileURL(proxyPath))).default;
}

throw e;
}
}
}

/**
* Calculates the line and column for an error offset in the content of a JSON file.
* @param location The offset error location from the beginning of the content.
* @param content The full content of the file containing the error.
* @returns An object containing the line and column
*/
function getJsonErrorLineColumn(offset: number, content: string) {
if (offset === 0) {
return { line: 1, column: 1 };
}

throw e;
let line = 0;
let position = 0;
// eslint-disable-next-line no-constant-condition
while (true) {
++line;

const nextNewline = content.indexOf('\n', position);
if (nextNewline === -1 || nextNewline > offset) {
break;
}

position = nextNewline + 1;
}

throw new Error('Proxy config file ' + proxyPath + ' does not exist.');
return { line, column: offset - position + 1 };
}

/**
Expand Down

0 comments on commit 6d0f99a

Please sign in to comment.