Skip to content
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions change/@griffel-transform-vm-error-fix.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "fix: wrap VM errors as host Error with filename context, defer CJS export assignments for IIFE patterns",
"packageName": "@griffel/transform",
"email": "olfedias@microsoft.com",
"dependentChangeType": "patch"
}
7 changes: 7 additions & 0 deletions change/@griffel-webpack-plugin-vm-mode.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "fix: switch webpack-plugin to vm evaluation mode",
"packageName": "@griffel/webpack-plugin",
"email": "olfedias@microsoft.com",
"dependentChangeType": "patch"
}
55 changes: 37 additions & 18 deletions packages/transform/src/evaluation/module.mts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@ let cache: Record<string, Module> = {};

const NOOP = () => {};

/** Checks if a value is an Error-like object (works across VM contexts where `instanceof Error` fails). */
function isError(e: unknown): e is Error {
return e != null && typeof e === 'object' && 'message' in e && 'stack' in e;
}

const createCustomDebug =
(depth: number) =>
(namespaces: string, arg1: unknown, ...args: unknown[]) => {
Expand Down Expand Up @@ -215,24 +220,38 @@ export class Module {
filename: this.filename,
});

script.runInContext(
vm.createContext({
clearImmediate: NOOP,
clearInterval: NOOP,
clearTimeout: NOOP,
setImmediate: NOOP,
setInterval: NOOP,
setTimeout: NOOP,
fetch: NOOP,
global,
process: mockProcess,
module: this,
exports: this.exports,
require: this.require,
__filename: this.filename,
__dirname: path.dirname(this.filename),
}),
);
try {
script.runInContext(
vm.createContext({
clearImmediate: NOOP,
clearInterval: NOOP,
clearTimeout: NOOP,
setImmediate: NOOP,
setInterval: NOOP,
setTimeout: NOOP,
fetch: NOOP,
global,
process: mockProcess,
module: this,
exports: this.exports,
require: this.require,
__filename: this.filename,
__dirname: path.dirname(this.filename),
}),
);
} catch (vmError: unknown) {
// Errors thrown inside vm.runInContext() use the VM context's Error constructor,
// so they fail `instanceof Error` in the host context (e.g. webpack wraps them as NonErrorEmittedError).
// Re-create as a host Error with filename context.
const message = isError(vmError) ? vmError.message : String(vmError);
const hostError = new Error(message);

if (isError(vmError)) {
hostError.stack = vmError.stack;
}

throw hostError;
}

EvalCache.set(cacheKey, text, this.exports);
}
Expand Down
61 changes: 60 additions & 1 deletion packages/transform/src/evaluation/module.test.mts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@ import type { TransformResolver } from './module.mjs';

const defaultRules = [{ action: 'ignore' as const }];
const defaultResolve: TransformResolver = (id, opts) => ({
path: (NativeModule as unknown as { _resolveFilename: (id: string, options: unknown) => string })._resolveFilename(id, opts),
path: (NativeModule as unknown as { _resolveFilename: (id: string, options: unknown) => string })._resolveFilename(
id,
opts,
),
builtin: false,
});

Expand All @@ -25,6 +28,62 @@ describe('Module', () => {
}
});

describe('evaluate', () => {
it('wraps VM errors as host Error with filename context', () => {
tmpDir = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'griffel-module-test-')));

const childFile = path.join(tmpDir, 'child.js');
fs.writeFileSync(childFile, 'const x = undefined;\nx.foo;');

const entryFile = path.join(tmpDir, 'entry.js');
fs.writeFileSync(entryFile, '');

const m = new Module(entryFile, defaultRules, defaultResolve);

try {
m.evaluate(`const child = require("./child.js");`, ['child']);
expect.unreachable('should have thrown');
} catch (e) {
// Must be a proper host Error instance (not a VM context Error that webpack wraps as NonErrorEmittedError)
expect(e).toBeInstanceOf(Error);

const err = e as Error;
// Replace machine-specific paths and line numbers so snapshots are stable across environments
const repoRoot = path.resolve(__dirname, '../../../..');
const normalize = (s: string) =>
s
.split(tmpDir).join('<tmpDir>')
.split(repoRoot).join('<repo>')
.replace(/:\d+(:\d+)?/g, ':<line>');

expect(normalize(err.message)).toMatchInlineSnapshot(
`"Cannot read properties of undefined (reading 'foo')"`,
);
expect(normalize(err.stack!)).toMatchInlineSnapshot(`
"<repo>/packages/transform/src/evaluation/module.mts:<line>
throw hostError;
^

<tmpDir>/child.js:<line>
x.foo;
^

TypeError: Cannot read properties of undefined (reading 'foo')
at <tmpDir>/child.js:<line>
at <tmpDir>/child.js:<line>
at Script.runInContext (node:vm:<line>)
at Module.evaluate (<repo>/packages/transform/src/evaluation/module.mts:<line>)
at require.Object.assign.ensure (<repo>/packages/transform/src/evaluation/module.mts:<line>)
at <tmpDir>/entry.js:<line>
at <tmpDir>/entry.js:<line>
at Script.runInContext (node:vm:<line>)
at Module.evaluate (<repo>/packages/transform/src/evaluation/module.mts:<line>)
at <repo>/packages/transform/src/evaluation/module.test.mts:<line>"
`);
}
});
});

describe('require (asset handling)', () => {
it('wraps non-JS/JSON requires with asset tags containing absolute path', () => {
tmpDir = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'griffel-module-test-')));
Expand Down
4 changes: 3 additions & 1 deletion packages/transform/src/transformSync.mts
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,9 @@ export function transformSync(sourceCode: string, options: TransformOptions): Tr
const functionKind = importedName as FunctionKinds;

if (node.arguments.length !== 1) {
throw new Error(`${functionKind}() function accepts only a single param`);
throw new Error(
`${functionKind}() function accepts only a single param, got ${node.arguments.length} in ${filename}`,
);
}

// Track the import specifier for rewriting (deduped by node start position)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
const obj: any = undefined;
export const color = obj.missingProp;
8 changes: 8 additions & 0 deletions packages/webpack-plugin/__fixtures__/vm-error-trace/code.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { makeStyles } from '@griffel/react';
import { color } from './broken';

export const useStyles = makeStyles({
root: {
color,
},
});
48 changes: 48 additions & 0 deletions packages/webpack-plugin/src/GriffelPlugin.test.mts
Original file line number Diff line number Diff line change
Expand Up @@ -433,4 +433,52 @@ describe('GriffelCSSExtractionPlugin', () => {
},
},
});

// Error reporting
// --------------------
it(
'includes the offending filename in VM evaluation errors',
async () => {
const fixturePath = path.resolve(__dirname, '..', '__fixtures__', 'vm-error-trace');
const entryPath = path.resolve(fixturePath, 'code.ts');

// Replace machine-specific paths and line numbers so snapshots are stable across environments
const repoRoot = path.resolve(__dirname, '../../..');
const normalize = (s: string) =>
s.split(repoRoot).join('<repo>').replace(/:\d+(:\d+)?/g, ':<line>');

let error: WebpackStatsError | undefined;

try {
await compileSourceWithWebpack(entryPath, {});
} catch (e) {
error = e as WebpackStatsError;
}

expect(error).toBeDefined();
expect(normalize(error!.message)).toMatchInlineSnapshot(`
"Module build failed (from ./webpackLoader.vitest.cjs):
<repo>/packages/transform/src/evaluation/module.mts:<line>
throw hostError;
^

<repo>/packages/webpack-plugin/__fixtures__/vm-error-trace/broken.ts:<line>
const color = obj.missingProp;
^

TypeError: Cannot read properties of undefined (reading 'missingProp')
at <repo>/packages/webpack-plugin/__fixtures__/vm-error-trace/broken.ts:<line>
at <repo>/packages/webpack-plugin/__fixtures__/vm-error-trace/broken.ts:<line>
at Script.runInContext (node:vm:<line>)
at Module.evaluate (<repo>/packages/transform/src/evaluation/module.mts:<line>)
at require.Object.assign.ensure (<repo>/packages/transform/src/evaluation/module.mts:<line>)
at <repo>/packages/webpack-plugin/__fixtures__/vm-error-trace/code.ts:<line>
at <repo>/packages/webpack-plugin/__fixtures__/vm-error-trace/code.ts:<line>
at Script.runInContext (node:vm:<line>)
at Module.evaluate (<repo>/packages/transform/src/evaluation/module.mts:<line>)
at vmEvaluator (<repo>/packages/transform/src/evaluation/vmEvaluator.mts:<line>)"
`);
},
15000,
);
});
2 changes: 1 addition & 1 deletion packages/webpack-plugin/src/webpackLoader.mts
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ function webpackLoader(
meta: {
filename: this.resourcePath,
step: 'transform' as const,
evaluationMode: 'ast' as const,
evaluationMode: 'vm' as const,
},
};
});
Expand Down
Loading