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.

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

Add React error overlay and HMR source maps #8034

Merged
merged 7 commits into from May 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
56 changes: 54 additions & 2 deletions packages/core/integration-tests/test/hmr.js
Expand Up @@ -10,6 +10,7 @@ import {
overlayFS,
sleep,
run,
request,
} from '@parcel/test-utils';
import WebSocket from 'ws';
import json5 from 'json5';
Expand Down Expand Up @@ -110,6 +111,7 @@ describe('hmr', function () {
return {
outputs: JSON.parse(JSON.stringify(outputs)),
reloaded,
bundleGraph,
};
}

Expand Down Expand Up @@ -157,10 +159,12 @@ describe('hmr', function () {
assert.equal(message.type, 'update');

// Figure out why output doesn't change...
let localAsset = message.assets.find(
asset => asset.output === 'exports.a = 5;\nexports.b = 5;\n',
let localAsset = message.assets.find(asset =>
asset.output.includes('exports.a = 5;\nexports.b = 5;\n'),
);
assert(!!localAsset);
assert(localAsset.output.includes('//# sourceMappingURL'));
assert(localAsset.output.includes('//# sourceURL'));
});

it('should emit an HMR update for all new dependencies along with the changed file', async function () {
Expand Down Expand Up @@ -331,6 +335,31 @@ describe('hmr', function () {

assert.equal(message.type, 'update');
});

it('should respond to requests for assets by id', async function () {
let port = await getPort();
let b = bundler(path.join(__dirname, '/input/index.js'), {
serveOptions: {port},
hmrOptions: {port},
inputFS: overlayFS,
config,
});

subscription = await b.watch();
let event = await getNextBuild(b);

let bundleGraph = nullthrows(event.bundleGraph);
let asset = nullthrows(bundleGraph.getBundles()[0].getMainEntry());
let contents = await request('/__parcel_hmr/' + asset.id, port);
let publicId = nullthrows(bundleGraph).getAssetPublicId(asset);
assert(
contents.startsWith(
`parcelHotUpdate['${publicId}'] = function (require, module, exports) {`,
),
);
assert(contents.includes('//# sourceMappingURL'));
assert(contents.includes('//# sourceURL'));
});
});

// TODO: add test for 4532 (`require` call in modified asset in child bundle where HMR runtime runs in parent bundle)
Expand Down Expand Up @@ -548,6 +577,29 @@ module.hot.dispose((data) => {
assert.notEqual(url.search, search);
});

it('should have correct source locations in errors', async function () {
let {outputs, bundleGraph} = await testHMRClient(
'hmr-accept-self',
() => {
return {
'local.js': 'output(new Error().stack);',
};
},
);

let asset = bundleGraph
.getBundles()[0]
.traverseAssets((asset, _, actions) => {
if (asset.filePath.endsWith('local.js')) {
actions.stop();
return asset;
}
});

let stack = outputs.pop();
assert(stack.includes('/__parcel_hmr/' + nullthrows(asset).id));
});

/*
it.skip('should accept HMR updates in the runtime after an initial error', async function() {
await fs.mkdirp(path.join(__dirname, '/input'));
Expand Down
28 changes: 1 addition & 27 deletions packages/core/integration-tests/test/server.js
Expand Up @@ -10,8 +10,8 @@ import {
outputFS,
overlayFS,
ncp,
request as get,
} from '@parcel/test-utils';
import http from 'http';
import https from 'https';
import getPort from 'get-port';
import type {BuildEvent} from '@parcel/types';
Expand All @@ -22,32 +22,6 @@ const config = path.join(
'./integration/custom-configs/.parcelrc-dev-server',
);

function get(file, port, client = http) {
return new Promise((resolve, reject) => {
// $FlowFixMe
client.get(
{
hostname: 'localhost',
port: port,
path: file,
rejectUnauthorized: false,
},
res => {
res.setEncoding('utf8');
let data = '';
res.on('data', c => (data += c));
res.on('end', () => {
if (res.statusCode !== 200) {
return reject({statusCode: res.statusCode, data});
}

resolve(data);
});
},
);
});
}

describe('server', function () {
let subscription;

Expand Down
34 changes: 34 additions & 0 deletions packages/core/test-utils/src/utils.js
Expand Up @@ -27,6 +27,8 @@ import nullthrows from 'nullthrows';
import {parser as postHtmlParse} from 'posthtml-parser';
import postHtml from 'posthtml';
import EventEmitter from 'events';
import http from 'http';
import https from 'https';

import {makeDeferredWithPromise, normalizeSeparators} from '@parcel/utils';
import _chalk from 'chalk';
Expand Down Expand Up @@ -734,6 +736,8 @@ function prepareBrowserContext(
},
URL,
Worker: createWorkerClass(bundle.filePath),
addEventListener() {},
removeEventListener() {},
},
globals,
);
Expand Down Expand Up @@ -1157,3 +1161,33 @@ export async function assertNoFilePathInCache(
}
}
}

export function request(
file: string,
port: number,
client: typeof http | typeof https = http,
): Promise<string> {
return new Promise((resolve, reject) => {
// $FlowFixMe
client.get(
{
hostname: 'localhost',
port: port,
path: file,
rejectUnauthorized: false,
},
res => {
res.setEncoding('utf8');
let data = '';
res.on('data', c => (data += c));
res.on('end', () => {
if (res.statusCode !== 200) {
return reject({statusCode: res.statusCode, data});
}

resolve(data);
});
},
);
});
}
21 changes: 17 additions & 4 deletions packages/core/utils/src/prettyDiagnostic.js
Expand Up @@ -10,10 +10,18 @@ import nullthrows from 'nullthrows';
// $FlowFixMe
import terminalLink from 'terminal-link';

export type FormattedCodeFrame = {|
location: string,
code: string,
|};

export type AnsiDiagnosticResult = {|
message: string,
stack: string,
/** A formatted string containing all code frames, including their file locations. */
codeframe: string,
/** A list of code frames with highlighted code and file locations separately. */
frames: Array<FormattedCodeFrame>,
Copy link
Member Author

Choose a reason for hiding this comment

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

A bit unfortunate, but couldn't think of a better way...

Copy link
Member

@101arrowz 101arrowz May 2, 2022

Choose a reason for hiding this comment

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

You could make codeframe a getter that processes frames in the default manner so there's less duplication. Beyond disgusting regex I don't think there's any other way to support rewriting paths than saving the frames array.

hints: Array<string>,
documentation: string,
|};
Expand All @@ -39,6 +47,7 @@ export default async function prettyDiagnostic(
(skipFormatting ? message : mdAnsi(message)),
stack: '',
codeframe: '',
frames: [],
hints: [],
documentation: '',
};
Expand Down Expand Up @@ -69,16 +78,20 @@ export default async function prettyDiagnostic(
});
}

result.codeframe +=
let location =
typeof filePath !== 'string'
? ''
: chalk.gray.underline(
`${filePath}:${highlights[0].start.line}:${highlights[0].start.column}\n`,
);
: `${filePath}:${highlights[0].start.line}:${highlights[0].start.column}`;
result.codeframe += location ? chalk.gray.underline(location) + '\n' : '';
result.codeframe += formattedCodeFrame;
if (codeFrame !== codeFrames[codeFrames.length - 1]) {
result.codeframe += '\n\n';
}

result.frames.push({
location,
code: formattedCodeFrame,
});
}
}

Expand Down
2 changes: 2 additions & 0 deletions packages/reporters/dev-server/package.json
Expand Up @@ -38,6 +38,8 @@
"connect": "^3.7.0",
"ejs": "^3.1.6",
"http-proxy-middleware": "^2.0.1",
"launch-editor": "^2.3.0",
"mime-types": "2.1.18",
"nullthrows": "^1.1.1",
"serve-handler": "^6.0.0",
"ws": "^7.0.0"
Expand Down
55 changes: 51 additions & 4 deletions packages/reporters/dev-server/src/HMRServer.js
@@ -1,16 +1,25 @@
// @flow

import type {BuildSuccessEvent, Dependency, PluginOptions} from '@parcel/types';
import type {
BuildSuccessEvent,
Dependency,
PluginOptions,
BundleGraph,
PackagedBundle,
Asset,
} from '@parcel/types';
import type {Diagnostic} from '@parcel/diagnostic';
import type {AnsiDiagnosticResult} from '@parcel/utils';
import type {ServerError, HMRServerOptions} from './types.js.flow';

import WebSocket from 'ws';
import invariant from 'assert';
import {ansiHtml, prettyDiagnostic, PromiseQueue} from '@parcel/utils';
import {HMR_ENDPOINT} from './Server';

export type HMRAsset = {|
id: string,
url: string,
type: string,
output: string,
envHash: string,
Expand All @@ -26,7 +35,7 @@ export type HMRMessage =
type: 'error',
diagnostics: {|
ansi: Array<AnsiDiagnosticResult>,
html: Array<AnsiDiagnosticResult>,
html: Array<$Rest<AnsiDiagnosticResult, {|codeframe: string|}>>,
|},
|};

Expand Down Expand Up @@ -81,7 +90,10 @@ export default class HMRServer {
return {
message: ansiHtml(d.message),
stack: ansiHtml(d.stack),
codeframe: ansiHtml(d.codeframe),
frames: d.frames.map(f => ({
location: f.location,
code: ansiHtml(f.code),
})),
hints: d.hints.map(hint => ansiHtml(hint)),
documentation: diagnostics[i].documentationURL ?? '',
};
Expand Down Expand Up @@ -141,9 +153,13 @@ export default class HMRServer {

return {
id: event.bundleGraph.getAssetPublicId(asset),
url: getSourceURL(event.bundleGraph, asset),
type: asset.type,
// No need to send the contents of non-JS assets to the client.
output: asset.type === 'js' ? await asset.getCode() : '',
output:
asset.type === 'js'
? await getHotAssetContents(event.bundleGraph, asset)
: '',
envHash: asset.env.id,
depsByBundle,
};
Expand Down Expand Up @@ -185,3 +201,34 @@ function getSpecifier(dep: Dependency): string {

return dep.specifier;
}

export async function getHotAssetContents(
bundleGraph: BundleGraph<PackagedBundle>,
asset: Asset,
): Promise<string> {
let output = await asset.getCode();
if (asset.type === 'js') {
let publicId = bundleGraph.getAssetPublicId(asset);
output = `parcelHotUpdate['${publicId}'] = function (require, module, exports) {${output}}`;
}

let sourcemap = await asset.getMap();
if (sourcemap) {
let sourcemapStringified = await sourcemap.stringify({
format: 'inline',
sourceRoot: '/__parcel_source_root/',
Copy link
Member Author

Choose a reason for hiding this comment

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

Somewhat related, but this source root seems to be displayed by react-error-overlay rather than either the original file path, or nothing. Not sure if there is a way to improve that?

// $FlowFixMe
fs: asset.fs,
});

invariant(typeof sourcemapStringified === 'string');
output += `\n//# sourceMappingURL=${sourcemapStringified}`;
output += `\n//# sourceURL=${getSourceURL(bundleGraph, asset)}\n`;
}

return output;
}

function getSourceURL(bundleGraph, asset) {
return HMR_ENDPOINT + asset.id;
}