From 8bab0bca3c1668afadd7c7e7261f934ee0b7ee18 Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Thu, 20 Apr 2023 16:21:40 -0400 Subject: [PATCH] [babel] Compile individual components Updates the Babel plugin so that we can individually compile components and skip over ones that have non-critical errors --- compiler/forget/src/Babel/BabelPlugin.ts | 212 +++++++++--------- .../forget/src/__tests__/compiler-test.ts | 22 +- .../disableMemoizeJsxElements-test.ts | 2 +- ...ror.file-has-non-critical-errors.expect.md | 54 +++++ .../error.file-has-non-critical-errors.js | 10 + .../test-utils/generateTestsFromFixtures.ts | 6 +- 6 files changed, 196 insertions(+), 110 deletions(-) create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.file-has-non-critical-errors.expect.md create mode 100644 compiler/forget/src/__tests__/fixtures/compiler/error.file-has-non-critical-errors.js diff --git a/compiler/forget/src/Babel/BabelPlugin.ts b/compiler/forget/src/Babel/BabelPlugin.ts index 7d5d556dc9..0380706acd 100644 --- a/compiler/forget/src/Babel/BabelPlugin.ts +++ b/compiler/forget/src/Babel/BabelPlugin.ts @@ -54,48 +54,67 @@ export default function ReactForgetBabelPlugin( fn: BabelCore.NodePath, pass: BabelPluginPass ): void { - const compiled = compile(fn, pass.opts.environment); + try { + const compiled = compile(fn, pass.opts.environment); - if (pass.opts.gating != null) { - // Rename existing function - if (fn.node.id == null) { - CompilerError.invariant( - "FunctionDeclaration must have a name", - fn.node.loc ?? GeneratedSource + if (pass.opts.gating != null) { + // Rename existing function + if (fn.node.id == null) { + CompilerError.invariant( + "FunctionDeclaration must have a name", + fn.node.loc ?? GeneratedSource + ); + } + const original = fn.node.id; + fn.node.id = addSuffix(fn.node.id, "_uncompiled"); + + // Rename and append compiled function + if (compiled.id == null) { + CompilerError.invariant( + "FunctionDeclaration must produce a name", + fn.node.loc ?? GeneratedSource + ); + } + compiled.id = addSuffix(compiled.id, "_forget"); + const compiledFn = fn.insertAfter(compiled)[0]; + compiledFn.skip(); + + // Build and append gating test + compiledFn.insertAfter( + buildGatingTest({ + originalFnDecl: fn, + compiledIdent: compiled.id, + originalIdent: original, + gating: pass.opts.gating, + }) ); + } else { + fn.replaceWith(compiled); } - const original = fn.node.id; - fn.node.id = addSuffix(fn.node.id, "_uncompiled"); - // Rename and append compiled function - if (compiled.id == null) { - CompilerError.invariant( - "FunctionDeclaration must produce a name", - fn.node.loc ?? GeneratedSource - ); + hasForgetCompiledCode = true; + } catch (err) { + if (pass.opts.logger && err) { + pass.opts.logger.logEvent("err", err); } - compiled.id = addSuffix(compiled.id, "_forget"); - const compiledFn = fn.insertAfter(compiled)[0]; - compiledFn.skip(); - - // Build and append gating test - compiledFn.insertAfter( - buildGatingTest({ - originalFnDecl: fn, - compiledIdent: compiled.id, - originalIdent: original, - gating: pass.opts.gating, - }) - ); - } else { - fn.replaceWith(compiled); + /** Always throw if the flag is enabled, otherwise we only throw if the error is critical + * (eg an invariant is broken, meaning the compiler may be buggy). See + * {@link CompilerError.isCritical} for mappings. + * */ + if ( + pass.opts.panicOnBailout || + !(err instanceof CompilerError) || + (err instanceof CompilerError && err.isCritical()) + ) { + throw err; + } else { + console.error(err); + } + } finally { + // We are generating a new FunctionDeclaration node, so we must skip over it or this + // traversal will loop infinitely. + fn.skip(); } - - hasForgetCompiledCode = true; - - // We are generating a new FunctionDeclaration node, so we must skip over it or this - // traversal will loop infinitely. - fn.skip(); } const visitor = { @@ -190,78 +209,59 @@ export default function ReactForgetBabelPlugin( return; } - try { - path.traverse(visitor, { - ...pass, - opts: { ...pass.opts, ...options }, - }); + path.traverse(visitor, { + ...pass, + opts: { ...pass.opts, ...options }, + }); - // If there isn't already an import of * as React, insert it so React.useMemoCache doesn't - // throw - if (hasForgetCompiledCode) { - let didInsertUseMemoCache = false; - let hasExistingReactImport = false; - path.traverse({ - MemberExpression(memberExprPath) { - const obj = memberExprPath.get("object"); - const prop = memberExprPath.get("property"); - if ( - obj.isIdentifier() && - obj.node.name === "React" && - prop.isIdentifier() && - prop.node.name === "unstable_useMemoCache" - ) { - didInsertUseMemoCache = true; - memberExprPath.stop(); - } - }, - ImportDeclaration(importDeclPath) { - if ( - importDeclPath.get("source").node.value === "react" && - importDeclPath.get("specifiers").length === 1 && - importDeclPath - .get("specifiers")[0] - .isImportNamespaceSpecifier() && - importDeclPath.get("specifiers")[0].get("local").node.name === - "React" - ) { - hasExistingReactImport = true; - importDeclPath.stop(); - } - }, - }); - if (didInsertUseMemoCache && !hasExistingReactImport) { - path.unshiftContainer( - "body", - t.importDeclaration( - [t.importNamespaceSpecifier(t.identifier("React"))], - t.stringLiteral("react") - ) - ); - } - if (options.gating != null) { - path.unshiftContainer( - "body", - buildImportForGatingModule(options.gating) - ); - } + // If there isn't already an import of * as React, insert it so React.useMemoCache doesn't + // throw + if (hasForgetCompiledCode) { + let didInsertUseMemoCache = false; + let hasExistingReactImport = false; + path.traverse({ + MemberExpression(memberExprPath) { + const obj = memberExprPath.get("object"); + const prop = memberExprPath.get("property"); + if ( + obj.isIdentifier() && + obj.node.name === "React" && + prop.isIdentifier() && + prop.node.name === "unstable_useMemoCache" + ) { + didInsertUseMemoCache = true; + memberExprPath.stop(); + } + }, + ImportDeclaration(importDeclPath) { + if ( + importDeclPath.get("source").node.value === "react" && + importDeclPath.get("specifiers").length === 1 && + importDeclPath + .get("specifiers")[0] + .isImportNamespaceSpecifier() && + importDeclPath.get("specifiers")[0].get("local").node.name === + "React" + ) { + hasExistingReactImport = true; + importDeclPath.stop(); + } + }, + }); + if (didInsertUseMemoCache && !hasExistingReactImport) { + path.unshiftContainer( + "body", + t.importDeclaration( + [t.importNamespaceSpecifier(t.identifier("React"))], + t.stringLiteral("react") + ) + ); } - } catch (err) { - if (options.logger && err) { - options.logger.logEvent("err", err); - } - /** Always throw if the flag is enabled, otherwise we only throw if the error is critical - * (eg an invariant is broken, meaning the compiler may be buggy). See - * {@link CompilerError.isCritical} for mappings. - * */ - if ( - options.panicOnBailout || - !(err instanceof CompilerError) || - (err instanceof CompilerError && err.isCritical()) - ) { - throw err; - } else { - console.error(err); + if (options.gating != null) { + path.unshiftContainer( + "body", + buildImportForGatingModule(options.gating) + ); } } }, diff --git a/compiler/forget/src/__tests__/compiler-test.ts b/compiler/forget/src/__tests__/compiler-test.ts index 1a8c24a5ca..b3715f5810 100644 --- a/compiler/forget/src/__tests__/compiler-test.ts +++ b/compiler/forget/src/__tests__/compiler-test.ts @@ -29,14 +29,20 @@ wasmFolder( ); describe("React Forget", () => { + const originalConsoleError = console.error; generateTestsFromFixtures( path.join(__dirname, "fixtures", "compiler"), (input, file, options) => { + const seenConsoleErrors: Array = []; let items: Array = []; let error: Error | null = null; if (options.debug) { toggleLogging(options.debug); } + // Mock console.error so we can record it in test output + console.error = jest.fn((...messages: Array) => { + seenConsoleErrors.push(...messages); + }); try { items.push({ js: runReactForgetBabelPlugin(input, file, options.language, { @@ -58,12 +64,23 @@ describe("React Forget", () => { }, logger: null, gating: options.gating, - panicOnBailout: true, + panicOnBailout: options.panicOnBailout, }).code, }); } catch (e) { error = e; } + + // Promote console errors so they can be recorded in fixture output + for (const consoleError of seenConsoleErrors) { + if (error != null) { + error.message = `${error.message}\n\n${consoleError}`; + } else { + error = new Error(consoleError); + error.name = "ConsoleError"; + } + } + let outputs: Array; const expectError = file.startsWith("error."); @@ -73,7 +90,7 @@ describe("React Forget", () => { `Expected an error to be thrown for fixture: '${file}', remove the 'error.' prefix if an error is not expected.` ); } else { - outputs = [formatErrorOutput(error)]; + outputs = [...formatOutput(items), formatErrorOutput(error)]; } } else { if (error !== null) { @@ -94,6 +111,7 @@ ${outputs.join("\n")} `; } ); + console.error = originalConsoleError; }); function formatErrorOutput(error: Error): string { diff --git a/compiler/forget/src/__tests__/disableMemoizeJsxElements-test.ts b/compiler/forget/src/__tests__/disableMemoizeJsxElements-test.ts index 26617b398e..1148bc2aed 100644 --- a/compiler/forget/src/__tests__/disableMemoizeJsxElements-test.ts +++ b/compiler/forget/src/__tests__/disableMemoizeJsxElements-test.ts @@ -54,7 +54,7 @@ describe("React Forget (Disable memoization of JSX elements)", () => { }, logger: null, gating: options.gating, - panicOnBailout: true, + panicOnBailout: options.panicOnBailout, }).code, }); } catch (e) { diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.file-has-non-critical-errors.expect.md b/compiler/forget/src/__tests__/fixtures/compiler/error.file-has-non-critical-errors.expect.md new file mode 100644 index 0000000000..0dbe427c7d --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.file-has-non-critical-errors.expect.md @@ -0,0 +1,54 @@ + +## Input + +```javascript +// @panicOnBailout false +function Bad() { + var x = 1; + return
{x}
; +} + +function Good() { + const x = 1; + return
{x}
; +} + +``` + +## Code + +```javascript +import * as React from "react"; // @panicOnBailout false +function Bad() { + var x = 1; + return
{x}
; +} + +function Good() { + const $ = React.unstable_useMemoCache(1); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 =
{1}
; + $[0] = t0; + } else { + t0 = $[0]; + } + return t0; +} + +``` + +## Error + +``` +[ReactForget] TodoError: (BuildHIR::lowerStatement) Handle var kinds in VariableDeclaration + 1 | // @panicOnBailout false + 2 | function Bad() { +> 3 | var x = 1; + | ^^^^^^^^^^ + 4 | return
{x}
; + 5 | } + 6 | +``` + + \ No newline at end of file diff --git a/compiler/forget/src/__tests__/fixtures/compiler/error.file-has-non-critical-errors.js b/compiler/forget/src/__tests__/fixtures/compiler/error.file-has-non-critical-errors.js new file mode 100644 index 0000000000..7aa1ea53d4 --- /dev/null +++ b/compiler/forget/src/__tests__/fixtures/compiler/error.file-has-non-critical-errors.js @@ -0,0 +1,10 @@ +// @panicOnBailout false +function Bad() { + var x = 1; + return
{x}
; +} + +function Good() { + const x = 1; + return
{x}
; +} diff --git a/compiler/forget/src/__tests__/test-utils/generateTestsFromFixtures.ts b/compiler/forget/src/__tests__/test-utils/generateTestsFromFixtures.ts index 3e8ef13ce4..ffa4de7c80 100644 --- a/compiler/forget/src/__tests__/test-utils/generateTestsFromFixtures.ts +++ b/compiler/forget/src/__tests__/test-utils/generateTestsFromFixtures.ts @@ -92,6 +92,7 @@ export default function generateTestsFromFixtures( let enableOnlyOnUseForgetDirective = false; let gating: GatingOptions | null = null; let inlineUseMemo = true; + let panicOnBailout = true; if (inputFile != null) { input = fs.readFileSync(inputFile, "utf8"); @@ -115,6 +116,9 @@ export default function generateTestsFromFixtures( if (lines[0]!.indexOf("@inlineUseMemo") !== -1) { inlineUseMemo = true; } + if (lines[0]!.indexOf("@panicOnBailout false") !== -1) { + panicOnBailout = false; + } } testCommand(basename, () => { @@ -127,7 +131,7 @@ export default function generateTestsFromFixtures( enableOnlyOnUseForgetDirective, gating, language: parseLanguage(input), - panicOnBailout: true, + panicOnBailout, }); } else { receivedOutput = "<>";