Skip to content

Commit

Permalink
Improve compiler error display for latest Chromium
Browse files Browse the repository at this point in the history
This commit addresses the issue of Chromium v126 and later not displaying
error messages correctly when the error object's `message` property uses
a getter. It refactors the code to utilize an immutable Error object with
recursive context, improves error message formatting and leverages the
`cause` property.

Changes:

- Refactor error wrapping internals to use an immutable error object,
  eliminating `message` getters.
- Utilize the `cause` property in contextual errors for enhanced error
  display in the console.
- Enhance message formatting with better indentation and listing.
- Improve clarity by renaming values thrown during validations.
  • Loading branch information
undergroundwires committed Jul 21, 2024
1 parent abe03ce commit b16e136
Show file tree
Hide file tree
Showing 15 changed files with 248 additions and 105 deletions.
2 changes: 1 addition & 1 deletion src/application/Parser/ApplicationParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ function validateCollectionsData(
) {
validator.assertNonEmptyCollection({
value: collections,
valueName: 'collections',
valueName: 'Collections',
});
}

Expand Down
4 changes: 2 additions & 2 deletions src/application/Parser/CategoryCollectionParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,14 @@ function validateCollection(
): void {
validator.assertObject({
value: content,
valueName: 'collection',
valueName: 'Collection',
allowedProperties: [
'os', 'scripting', 'actions', 'functions',
],
});
validator.assertNonEmptyCollection({
value: content.actions,
valueName: '"actions" in collection',
valueName: '\'actions\' in collection',
});
}

Expand Down
118 changes: 96 additions & 22 deletions src/application/Parser/Common/ContextualError.ts
Original file line number Diff line number Diff line change
@@ -1,42 +1,116 @@
import { CustomError } from '@/application/Common/CustomError';
import { indentText } from '@/application/Common/Text/IndentText';

export interface ErrorWithContextWrapper {
(
error: Error,
innerError: Error,
additionalContext: string,
): Error;
}

export const wrapErrorWithAdditionalContext: ErrorWithContextWrapper = (
error: Error,
additionalContext: string,
innerError,
additionalContext,
) => {
return (error instanceof ContextualError ? error : new ContextualError(error))
.withAdditionalContext(additionalContext);
if (!additionalContext) {
throw new Error('Missing additional context');
}
return new ContextualError({
innerError,
additionalContext,
});
};

/* AggregateError is similar but isn't well-serialized or displayed by browsers */
/**
* Class for building a detailed error trace.
*
* Alternatives considered:
* - `AggregateError`:
* Similar but not well-serialized or displayed by browsers such as Chromium (last tested v126).
* - `cause` property:
* Not displayed by all browsers (last tested v126).
* Reference: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/cause
*
* This is immutable where the constructor sets the values because using getter functions such as
* `get cause()`, `get message()` does not work on Chromium (last tested v126), but works fine on
* Firefox (last tested v127).
*/
class ContextualError extends CustomError {
private readonly additionalContext = new Array<string>();
constructor(public readonly context: ErrorContext) {
super(
generateDetailedErrorMessageWithContext(context),
{
cause: context.innerError,
},
);
}
}

constructor(
public readonly innerError: Error,
) {
super();
interface ErrorContext {
readonly innerError: Error;
readonly additionalContext: string;
}

function generateDetailedErrorMessageWithContext(
context: ErrorContext,
): string {
return [
'\n',
// Display the current error message first, then the root cause.
// This prevents repetitive main messages for errors with a `cause:` chain,
// aligning with browser error display conventions.
context.additionalContext,
'\n',
'Error Trace (starting from root cause):',
indentText(
formatErrorTrace(
// Displaying contexts from the top frame (deepest, most recent) aligns with
// common debugger/compiler standard.
extractErrorTraceAscendingFromDeepest(context),
),
),
'\n',
].join('\n');
}

function extractErrorTraceAscendingFromDeepest(
context: ErrorContext,
): string[] {
const originalError = findRootError(context.innerError);
const contextsDescendingFromMostRecent: string[] = [
context.additionalContext,
...gatherContextsFromErrorChain(context.innerError),
originalError.toString(),
];
const contextsAscendingFromDeepest = contextsDescendingFromMostRecent.reverse();
return contextsAscendingFromDeepest;
}

function findRootError(error: Error): Error {
if (error instanceof ContextualError) {
return findRootError(error.context.innerError);
}
return error;
}

public withAdditionalContext(additionalContext: string): this {
this.additionalContext.push(additionalContext);
return this;
function gatherContextsFromErrorChain(
error: Error,
accumulatedContexts: string[] = [],
): string[] {
if (error instanceof ContextualError) {
accumulatedContexts.push(error.context.additionalContext);
return gatherContextsFromErrorChain(error.context.innerError, accumulatedContexts);
}
return accumulatedContexts;
}

public get message(): string { // toString() is not used when Chromium logs it on console
return [
'\n',
this.innerError.message,
'\n',
'Additional context:',
...this.additionalContext.map((context, index) => `${index + 1}: ${context}`),
].join('\n');
function formatErrorTrace(
errorMessages: readonly string[],
): string {
if (errorMessages.length === 1) {
return errorMessages[0];
}
return errorMessages
.map((context, index) => `${index + 1}.${indentText(context)}`)
.join('\n');
}
4 changes: 2 additions & 2 deletions src/application/Parser/Common/TypeValidator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ function assertArray(
valueName: string,
): asserts value is Array<unknown> {
if (!isArray(value)) {
throw new Error(`'${valueName}' should be of type 'array', but is of type '${typeof value}'.`);
throw new Error(`${valueName} should be of type 'array', but is of type '${typeof value}'.`);
}
}

Expand All @@ -117,7 +117,7 @@ function assertString(
valueName: string,
): asserts value is string {
if (!isString(value)) {
throw new Error(`'${valueName}' should be of type 'string', but is of type '${typeof value}'.`);
throw new Error(`${valueName} should be of type 'string', but is of type '${typeof value}'.`);
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/application/Parser/Executable/CategoryParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ function ensureValidCategory(
});
validator.assertType((v) => v.assertObject({
value: category,
valueName: category.category ?? 'category',
valueName: `Category '${category.category}'` ?? 'Category',
allowedProperties: [
'docs', 'children', 'category',
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export const createFunctionCallArgument: FunctionCallArgumentFactory = (
utilities.validateParameterName(parameterName);
utilities.typeValidator.assertNonEmptyString({
value: argumentValue,
valueName: `Missing argument value for the parameter "${parameterName}".`,
valueName: `Function parameter '${parameterName}'`,
});
return {
parameterName,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ function getCallSequence(calls: FunctionCallsData, validator: TypeValidator): Fu
if (isArray(calls)) {
validator.assertNonEmptyCollection({
value: calls,
valueName: 'function call sequence',
valueName: 'Function call sequence',
});
return calls as FunctionCallData[];
}
Expand All @@ -56,7 +56,7 @@ function parseFunctionCall(
): FunctionCall {
utilities.typeValidator.assertObject({
value: call,
valueName: 'function call',
valueName: 'Function call',
allowedProperties: ['function', 'parameters'],
});
const callArgs = parseArgs(call.parameters, utilities.createCallArgument);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export const validateParameterName = (
) => {
typeValidator.assertNonEmptyString({
value: parameterName,
valueName: 'parameter name',
valueName: 'Parameter name',
rule: {
expectedMatch: /^[0-9a-zA-Z]+$/,
errorMessage: `parameter name must be alphanumeric but it was "${parameterName}".`,
Expand Down
2 changes: 1 addition & 1 deletion src/application/Parser/Executable/Script/ScriptParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ function validateScript(
): asserts script is NonNullable<ScriptData> {
validator.assertType((v) => v.assertObject<CallScriptData & CodeScriptData>({
value: script,
valueName: script.name ?? 'script',
valueName: `Script '${script.name}'` ?? 'Script',
allowedProperties: [
'name', 'recommend', 'code', 'revertCode', 'call', 'docs',
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ function validateData(
): void {
validator.assertObject({
value: data,
valueName: 'scripting definition',
valueName: 'Scripting definition',
allowedProperties: ['language', 'fileExtension', 'startCode', 'endCode'],
});
}
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/application/Parser/ApplicationParser.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ describe('ApplicationParser', () => {
const data = [new CollectionDataStub()];
const expectedAssertion: NonEmptyCollectionAssertion = {
value: data,
valueName: 'collections',
valueName: 'Collections',
};
const validator = new TypeValidatorStub();
const sut = new ApplicationParserBuilder()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ describe('CategoryCollectionParser', () => {
const data = new CollectionDataStub();
const expectedAssertion: ObjectAssertion<CollectionData> = {
value: data,
valueName: 'collection',
valueName: 'Collection',
allowedProperties: [
'os', 'scripting', 'actions', 'functions',
],
Expand All @@ -48,7 +48,7 @@ describe('CategoryCollectionParser', () => {
const actions = [getCategoryStub('test1'), getCategoryStub('test2')];
const expectedAssertion: NonEmptyCollectionAssertion = {
value: actions,
valueName: '"actions" in collection',
valueName: '\'actions\' in collection',
};
const validator = new TypeValidatorStub();
const context = new TestContext()
Expand Down
Loading

0 comments on commit b16e136

Please sign in to comment.