From 62e6f30af2fb4d2fca73c55ca8d605615c6d27cb Mon Sep 17 00:00:00 2001 From: Titus Wormer Date: Mon, 25 Oct 2021 18:40:18 +0200 Subject: [PATCH] Add better errors when referencing missing components MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, React or other JSX runtimes threw rather hard to read errors when a component was undefined (because it wasn’t imported, passed, or provided), essentially only pointing to *something* missing. Now we throw proper errors when a component is missing at runtime, including what exact component (or object) is undefined. In addition, this adds a `development` option, which defaults to `false` but can be configured explicitly or turned on with `NODE_ENV=development`. When it’s `true`, the exact place that references the missing component or object, and which file did that, is included in the error message. Related-to: mdx-js/mdx#1775. --- lib/condition.browser.js | 1 + lib/condition.js | 3 + lib/core.js | 4 +- lib/plugin/recma-jsx-build.js | 12 +- lib/plugin/recma-jsx-rewrite.js | 231 ++++++++++++++---- lib/util/estree-util-to-binary-addition.js | 23 ++ .../estree-util-to-id-or-member-expression.js | 64 +++++ package.json | 6 + readme.md | 73 +++++- test/core.js | 51 +++- test/esm-loader.js | 2 +- 11 files changed, 412 insertions(+), 58 deletions(-) create mode 100644 lib/condition.browser.js create mode 100644 lib/condition.js create mode 100644 lib/util/estree-util-to-binary-addition.js create mode 100644 lib/util/estree-util-to-id-or-member-expression.js diff --git a/lib/condition.browser.js b/lib/condition.browser.js new file mode 100644 index 0000000..faf5737 --- /dev/null +++ b/lib/condition.browser.js @@ -0,0 +1 @@ +export const development = false diff --git a/lib/condition.js b/lib/condition.js new file mode 100644 index 0000000..5bf0a44 --- /dev/null +++ b/lib/condition.js @@ -0,0 +1,3 @@ +import process from 'node:process' + +export const development = process.env.NODE_ENV === 'development' diff --git a/lib/core.js b/lib/core.js index c2aed9e..b4caa66 100644 --- a/lib/core.js +++ b/lib/core.js @@ -31,6 +31,7 @@ import {rehypeRemoveRaw} from './plugin/rehype-remove-raw.js' import {remarkMarkAndUnravel} from './plugin/remark-mark-and-unravel.js' import {remarkMdx} from './plugin/remark-mdx.js' import {nodeTypes} from './node-types.js' +import {development as defaultDevelopment} from './condition.js' /** * Pipeline to: @@ -44,6 +45,7 @@ import {nodeTypes} from './node-types.js' */ export function createProcessor(options = {}) { const { + development = defaultDevelopment, jsx, format, outputFormat, @@ -81,7 +83,7 @@ export function createProcessor(options = {}) { pipeline .use(rehypeRecma) .use(recmaDocument, {...rest, outputFormat}) - .use(recmaJsxRewrite, {providerImportSource, outputFormat}) + .use(recmaJsxRewrite, {development, providerImportSource, outputFormat}) if (!jsx) { pipeline.use(recmaJsxBuild, {outputFormat}) diff --git a/lib/plugin/recma-jsx-build.js b/lib/plugin/recma-jsx-build.js index b28f6a3..0eacfc0 100644 --- a/lib/plugin/recma-jsx-build.js +++ b/lib/plugin/recma-jsx-build.js @@ -7,6 +7,7 @@ import {buildJsx} from 'estree-util-build-jsx' import {specifiersToDeclarations} from '../util/estree-util-specifiers-to-declarations.js' +import {toIdOrMemberExpression} from '../util/estree-util-to-id-or-member-expression.js' /** * A plugin to build JSX into function calls. @@ -33,13 +34,10 @@ export function recmaJsxBuild(options = {}) { tree.body[0] = { type: 'VariableDeclaration', kind: 'const', - declarations: specifiersToDeclarations(tree.body[0].specifiers, { - type: 'MemberExpression', - object: {type: 'Identifier', name: 'arguments'}, - property: {type: 'Literal', value: 0}, - computed: true, - optional: false - }) + declarations: specifiersToDeclarations( + tree.body[0].specifiers, + toIdOrMemberExpression(['arguments', 0]) + ) } } } diff --git a/lib/plugin/recma-jsx-rewrite.js b/lib/plugin/recma-jsx-rewrite.js index 1a8dd98..b83ea29 100644 --- a/lib/plugin/recma-jsx-rewrite.js +++ b/lib/plugin/recma-jsx-rewrite.js @@ -5,7 +5,6 @@ * @typedef {import('estree-jsx').ImportSpecifier} ImportSpecifier * @typedef {import('estree-jsx').JSXElement} JSXElement * @typedef {import('estree-jsx').JSXIdentifier} JSXIdentifier - * @typedef {import('estree-jsx').JSXMemberExpression} JSXMemberExpression * @typedef {import('estree-jsx').JSXNamespacedName} JSXNamespacedName * @typedef {import('estree-jsx').ModuleDeclaration} ModuleDeclaration * @typedef {import('estree-jsx').Program} Program @@ -13,6 +12,7 @@ * @typedef {import('estree-jsx').Statement} Statement * @typedef {import('estree-jsx').VariableDeclarator} VariableDeclarator * @typedef {import('estree-jsx').ObjectPattern} ObjectPattern + * @typedef {import('estree-jsx').Identifier} Identifier * * @typedef {import('estree-walker').SyncHandler} WalkHandler * @@ -20,19 +20,30 @@ * * @typedef RecmaJsxRewriteOptions * @property {'program'|'function-body'} [outputFormat='program'] Whether to use an import statement or `arguments[0]` to get the provider + * @property {boolean} [development=false] Whether to add extra information to error messages in generated code (can also be passed in Node.js by setting `NODE_ENV=development`) * @property {string} [providerImportSource] Place to import a provider from * * @typedef StackEntry * @property {Array.} objects * @property {Array.} components * @property {Array.} tags + * @property {Record.} references * @property {ESFunction} node */ +import {stringifyPosition} from 'unist-util-stringify-position' +import {positionFromEstree} from 'unist-util-position-from-estree' import {name as isIdentifierName} from 'estree-util-is-identifier-name' import {walk} from 'estree-walker' import {analyze} from 'periscopic' import {specifiersToDeclarations} from '../util/estree-util-specifiers-to-declarations.js' +import { + toIdOrMemberExpression, + toJsxIdOrMemberExpression +} from '../util/estree-util-to-id-or-member-expression.js' +import {toBinaryAddition} from '../util/estree-util-to-binary-addition.js' + +const own = {}.hasOwnProperty /** * A plugin that rewrites JSX in functions to accept components as @@ -44,15 +55,17 @@ import {specifiersToDeclarations} from '../util/estree-util-specifiers-to-declar * @type {import('unified').Plugin<[RecmaJsxRewriteOptions]|[], Program>} */ export function recmaJsxRewrite(options = {}) { - const {providerImportSource, outputFormat} = options + const {development, providerImportSource, outputFormat} = options - return (tree) => { + return (tree, file) => { // Find everything that’s defined in the top-level scope. const scopeInfo = analyze(tree) /** @type {Array.} */ const fnStack = [] /** @type {boolean|undefined} */ let importProvider + /** @type {boolean|undefined} */ + let createErrorHelper /** @type {Scope|null} */ let currentScope @@ -65,7 +78,13 @@ export function recmaJsxRewrite(options = {}) { node.type === 'FunctionExpression' || node.type === 'ArrowFunctionExpression' ) { - fnStack.push({objects: [], components: [], tags: [], node}) + fnStack.push({ + objects: [], + components: [], + tags: [], + references: {}, + node + }) } let fnScope = fnStack[0] @@ -100,11 +119,22 @@ export function recmaJsxRewrite(options = {}) { // ``, ``, ``. if (name.type === 'JSXMemberExpression') { + /** @type {string[]} */ + const ids = [] // Find the left-most identifier. - while (name.type === 'JSXMemberExpression') name = name.object + while (name.type === 'JSXMemberExpression') { + ids.unshift(name.property.name) + name = name.object + } + ids.unshift(name.name) + const fullId = ids.join('.') const id = name.name + if (!own.call(fnScope.references, fullId)) { + fnScope.references[fullId] = {node, component: true} + } + if (!fnScope.objects.includes(id) && !inScope(currentScope, id)) { fnScope.objects.push(id) } @@ -120,11 +150,16 @@ export function recmaJsxRewrite(options = {}) { else if (isIdentifierName(name.name) && !/^[a-z]/.test(name.name)) { const id = name.name - if ( - !fnScope.components.includes(id) && - !inScope(currentScope, id) - ) { - fnScope.components.push(id) + if (!inScope(currentScope, id)) { + // No need to add an error for an undefined layout — we use an + // `if` later. + if (id !== 'MDXLayout' && !own.call(fnScope.references, id)) { + fnScope.references[id] = {node, component: true} + } + + if (!fnScope.components.includes(id)) { + fnScope.components.push(id) + } } } // @ts-expect-error Allow fields passed through from mdast through hast to @@ -147,11 +182,10 @@ export function recmaJsxRewrite(options = {}) { } if (node.closingElement) { - node.closingElement.name = { - type: 'JSXMemberExpression', - object: {type: 'JSXIdentifier', name: '_components'}, - property: {type: 'JSXIdentifier', name: id} - } + node.closingElement.name = toJsxIdOrMemberExpression([ + '_components', + id + ]) } } } @@ -203,6 +237,26 @@ export function recmaJsxRewrite(options = {}) { } } + /** @type {string} */ + let key + + // Add partials (so for `x.y.z` it’d generate `x` and `x.y` too). + for (key in scope.references) { + if (own.call(scope.references, key)) { + const parts = key.split('.') + let index = 0 + while (++index < parts.length) { + const partial = parts.slice(0, index).join('.') + if (!own.call(scope.references, partial)) { + scope.references[partial] = { + node: scope.references[key].node, + component: false + } + } + } + } + } + if (defaults.length > 0 || actual.length > 0) { if (providerImportSource) { importProvider = true @@ -220,13 +274,7 @@ export function recmaJsxRewrite(options = {}) { isNamedFunction(scope.node, 'MDXContent') || isNamedFunction(scope.node, '_createMdxContent') ) { - parameters.push({ - type: 'MemberExpression', - object: {type: 'Identifier', name: 'props'}, - property: {type: 'Identifier', name: 'components'}, - computed: false, - optional: false - }) + parameters.push(toIdOrMemberExpression(['props', 'components'])) } if (defaults.length > 0 || parameters.length > 1) { @@ -242,13 +290,7 @@ export function recmaJsxRewrite(options = {}) { parameters.length > 1 ? { type: 'CallExpression', - callee: { - type: 'MemberExpression', - object: {type: 'Identifier', name: 'Object'}, - property: {type: 'Identifier', name: 'assign'}, - computed: false, - optional: false - }, + callee: toIdOrMemberExpression(['Object', 'assign']), arguments: parameters, optional: false } @@ -316,11 +358,55 @@ export function recmaJsxRewrite(options = {}) { } } - fn.body.body.unshift({ - type: 'VariableDeclaration', - kind: 'const', - declarations - }) + /** @type {Statement[]} */ + const statements = [ + { + type: 'VariableDeclaration', + kind: 'const', + declarations + } + ] + + const references = Object.keys(scope.references).sort() + let index = -1 + while (++index < references.length) { + const id = references[index] + const info = scope.references[id] + const place = stringifyPosition(positionFromEstree(info.node)) + /** @type {Expression[]} */ + const parameters = [ + {type: 'Literal', value: id}, + {type: 'Literal', value: info.component} + ] + + createErrorHelper = true + + if (development && place !== '1:1-1:1') { + parameters.push({type: 'Literal', value: place}) + } + + statements.push({ + type: 'IfStatement', + test: { + type: 'UnaryExpression', + operator: '!', + prefix: true, + argument: toIdOrMemberExpression(id.split('.')) + }, + consequent: { + type: 'ExpressionStatement', + expression: { + type: 'CallExpression', + callee: {type: 'Identifier', name: '_missingMdxReference'}, + arguments: parameters, + optional: false + } + }, + alternate: null + }) + } + + fn.body.body.unshift(...statements) } fnStack.pop() @@ -334,6 +420,72 @@ export function recmaJsxRewrite(options = {}) { createImportProvider(providerImportSource, outputFormat) ) } + + // If potentially missing components are used. + if (createErrorHelper) { + /** @type {Expression[]} */ + const message = [ + {type: 'Literal', value: 'Expected '}, + { + type: 'ConditionalExpression', + test: {type: 'Identifier', name: 'component'}, + consequent: {type: 'Literal', value: 'component'}, + alternate: {type: 'Literal', value: 'object'} + }, + {type: 'Literal', value: ' `'}, + {type: 'Identifier', name: 'id'}, + { + type: 'Literal', + value: + '` to be defined: you likely forgot to import, pass, or provide it.' + } + ] + + /** @type {Identifier[]} */ + const parameters = [ + {type: 'Identifier', name: 'id'}, + {type: 'Identifier', name: 'component'} + ] + + if (development) { + message.push({ + type: 'ConditionalExpression', + test: {type: 'Identifier', name: 'place'}, + consequent: toBinaryAddition([ + {type: 'Literal', value: '\nIt’s referenced in your code at `'}, + {type: 'Identifier', name: 'place'}, + { + type: 'Literal', + value: (file.path ? '` in `' + file.path : '') + '`' + } + ]), + alternate: {type: 'Literal', value: ''} + }) + + parameters.push({type: 'Identifier', name: 'place'}) + } + + tree.body.push({ + type: 'FunctionDeclaration', + id: {type: 'Identifier', name: '_missingMdxReference'}, + generator: false, + async: false, + params: parameters, + body: { + type: 'BlockStatement', + body: [ + { + type: 'ThrowStatement', + argument: { + type: 'NewExpression', + callee: {type: 'Identifier', name: 'Error'}, + arguments: [toBinaryAddition(message)] + } + } + ] + } + }) + } } } @@ -356,13 +508,10 @@ function createImportProvider(providerImportSource, outputFormat) { ? { type: 'VariableDeclaration', kind: 'const', - declarations: specifiersToDeclarations(specifiers, { - type: 'MemberExpression', - object: {type: 'Identifier', name: 'arguments'}, - property: {type: 'Literal', value: 0}, - computed: true, - optional: false - }) + declarations: specifiersToDeclarations( + specifiers, + toIdOrMemberExpression(['arguments', 0]) + ) } : { type: 'ImportDeclaration', diff --git a/lib/util/estree-util-to-binary-addition.js b/lib/util/estree-util-to-binary-addition.js new file mode 100644 index 0000000..f1496da --- /dev/null +++ b/lib/util/estree-util-to-binary-addition.js @@ -0,0 +1,23 @@ +/** + * @typedef {import('estree-jsx').Expression} Expression + */ + +/** + * @param {Expression[]} expressions + */ +export function toBinaryAddition(expressions) { + let index = -1 + /** @type {Expression|undefined} */ + let left + + while (++index < expressions.length) { + const right = expressions[index] + left = left ? {type: 'BinaryExpression', left, operator: '+', right} : right + } + + // Just for types. + /* c8 ignore next */ + if (!left) throw new Error('Expected non-empty `expressions` to be passed') + + return left +} diff --git a/lib/util/estree-util-to-id-or-member-expression.js b/lib/util/estree-util-to-id-or-member-expression.js new file mode 100644 index 0000000..e1f6122 --- /dev/null +++ b/lib/util/estree-util-to-id-or-member-expression.js @@ -0,0 +1,64 @@ +/** + * @typedef {import('estree-jsx').Identifier} Identifier + * @typedef {import('estree-jsx').Literal} Literal + * @typedef {import('estree-jsx').JSXIdentifier} JSXIdentifier + * @typedef {import('estree-jsx').MemberExpression} MemberExpression + * @typedef {import('estree-jsx').JSXMemberExpression} JSXMemberExpression + */ + +import {name as isIdentifierName} from 'estree-util-is-identifier-name' + +export const toIdOrMemberExpression = toIdOrMemberExpressionFactory( + 'Identifier', + 'MemberExpression' +) + +export const toJsxIdOrMemberExpression = + // @ts-expect-error: fine + /** @type {(ids: Array.) => JSXIdentifier|JSXMemberExpression)} */ + (toIdOrMemberExpressionFactory('JSXIdentifier', 'JSXMemberExpression')) + +/** + * @param {string} [idType] + * @param {string} [memberType] + */ +function toIdOrMemberExpressionFactory(idType, memberType) { + return toIdOrMemberExpression + /** + * @param {Array.} ids + * @returns {Identifier|MemberExpression} + */ + function toIdOrMemberExpression(ids) { + let index = -1 + /** @type {Identifier|Literal|MemberExpression|undefined} */ + let object + + while (++index < ids.length) { + const name = ids[index] + /** @type {Identifier|Literal} */ + // @ts-expect-error: JSX is fine. + const id = + typeof name === 'string' && isIdentifierName(name) + ? {type: idType, name} + : {type: 'Literal', value: name} + // @ts-expect-error: JSX is fine. + object = object + ? { + type: memberType, + object, + property: id, + computed: id.type === 'Literal', + optional: false + } + : id + } + + // Just for types. + /* c8 ignore next 3 */ + if (!object) throw new Error('Expected non-empty `ids` to be passed') + if (object.type === 'Literal') + throw new Error('Expected identifier as left-most value') + + return object + } +} diff --git a/package.json b/package.json index b7a9313..8749599 100644 --- a/package.json +++ b/package.json @@ -23,6 +23,12 @@ "main": "index.js", "sideEffects": false, "types": "./index.d.ts", + "browser": { + "./lib/condition.js": "./lib/condition.browser.js" + }, + "react-native": { + "./lib/condition.js": "./lib/condition.browser.js" + }, "files": [ "lib/", "complex-types.d.ts", diff --git a/readme.md b/readme.md index 32d76d6..c9d6bd1 100644 --- a/readme.md +++ b/readme.md @@ -480,6 +480,77 @@ export default MDXContent +###### `options.development` + +Whether to add extra information to error messages in generated code +(`boolean?`, default: `false`). +The default can be set to `true` in Node.js through environment variables: set +`NODE_ENV=development`. + +
+Example + +Say we had some MDX that references a component that can be passed or provided +at runtime: + +```mdx +**Note**: some stuff. +``` + +And a module to evaluate that: + +```js +import {promises as fs} from 'node:fs' +import * as runtime from 'react/jsx-runtime' +import {evaluate} from 'xdm' + +main() + +async function main() { + const path = 'example.mdx' + const value = await fs.readFile(path) + const MDXContent = (await evaluate({path, value}, runtime)).default + console.log(MDXContent()) +} +``` + +Running that would normally (production) yield: + +```txt +Error: Expected component `NoteIcon` to be defined: you likely forgot to import, pass, or provide it. + at _missingMdxReference (eval at run (…/xdm/lib/run.js:18:10), :27:9) + at _createMdxContent (eval at run (…/xdm/lib/run.js:18:10), :15:20) + at MDXContent (eval at run (…/xdm/lib/run.js:18:10), :9:9) + at main (…/example.js:11:15) +``` + +But if we change add `development: true` to our example: + +```diff +@@ -7,6 +7,6 @@ main() + async function main() { + const path = 'example.mdx' + const value = await fs.readFile(path) +- const MDXContent = (await evaluate({path, value}, runtime)).default ++ const MDXContent = (await evaluate({path, value}, {development: true, ...runtime})).default + console.log(MDXContent({})) + } +``` + +And we’d run it again, we’d get: + +```txt +Error: Expected component `NoteIcon` to be defined: you likely forgot to import, pass, or provide it. +It’s referenced in your code at `1:9-1:21` in `example.mdx` +provide it. + at _missingMdxReference (eval at run (…/xdm/lib/run.js:18:10), :27:9) + at _createMdxContent (eval at run (…/xdm/lib/run.js:18:10), :15:20) + at MDXContent (eval at run (…/xdm/lib/run.js:18:10), :9:9) + at main (…/example.js:11:15) +``` + +
+ ###### `options.SourceMapGenerator` The `SourceMapGenerator` class from [`source-map`][source-map] (optional). @@ -901,7 +972,7 @@ To pass options, you can make your own loader, such as this `my-loader.js`: ```js import {createLoader} from 'xdm/esm-loader.js' -// Load is for Node 17+, the rest for 12, 14, 16. +// Load is for Node 17+, the rest for 12-16. const {load, getFormat, transformSource} = createLoader(/* Options… */) export {load, getFormat, transformSource} diff --git a/test/core.js b/test/core.js index d338563..31167f1 100644 --- a/test/core.js +++ b/test/core.js @@ -476,8 +476,6 @@ export default function Layout({children}) { ) } - console.log('\nnote: the next warning is expected!\n') - try { renderToStaticMarkup(React.createElement(await run(compileSync('')))) t.fail() @@ -485,7 +483,7 @@ export default function Layout({children}) { const exception = /** @type {Error} */ (error) t.match( exception.message, - /Element type is invalid/, + /Expected component `X` to be defined: you likely forgot to import, pass, or provide it./, 'should throw if a required component is not passed' ) } @@ -497,11 +495,47 @@ export default function Layout({children}) { const exception = /** @type {Error} */ (error) t.match( exception.message, - /Cannot read propert/, + /Expected object `a` to be defined: you likely forgot to import, pass, or provide it/, 'should throw if a required member is not passed' ) } + try { + renderToStaticMarkup( + React.createElement(await run(compileSync('', {development: true}))) + ) + t.fail() + } catch (/** @type {unknown} */ error) { + const exception = /** @type {Error} */ (error) + t.match( + exception.message, + /It’s referenced in your code at `1:1-1:6/, + 'should pass more info to errors w/ `development: true`' + ) + } + + try { + renderToStaticMarkup( + React.createElement( + await run( + compileSync( + {value: 'asd ', path: 'folder/example.mdx'}, + {development: true} + ) + ) + ) + ) + t.fail() + } catch (/** @type {unknown} */ error) { + console.log(error) + const exception = /** @type {Error} */ (error) + t.match( + exception.message, + /It’s referenced in your code at `1:5-1:12` in `folder\/example.mdx`/, + 'should show what file contains the error w/ `development: true`, and `path`' + ) + } + t.equal( renderToStaticMarkup( React.createElement( @@ -525,8 +559,6 @@ export default function Layout({children}) { 'should support setting components through context with a `providerImportSource`' ) - console.log('\nnote: the next warning is expected!\n') - try { renderToStaticMarkup( React.createElement( @@ -538,7 +570,7 @@ export default function Layout({children}) { const exception = /** @type {Error} */ (error) t.match( exception.message, - /Element type is invalid/, + /Expected component `X` to be defined: you likely forgot to import, pass, or provide it/, 'should throw if a required component is not passed or given to `MDXProvider`' ) } @@ -747,10 +779,15 @@ test('jsx', async (t) => { ' return MDXLayout ? <_createMdxContent /> : _createMdxContent();', ' function _createMdxContent() {', ' const {c} = props.components || ({});', + ' if (!c) _missingMdxReference("c", false);', + ' if (!c.d) _missingMdxReference("c.d", true);', ' return <><>;', ' }', '}', 'export default MDXContent;', + 'function _missingMdxReference(id, component) {', + ' throw new Error("Expected " + (component ? "component" : "object") + " `" + id + "` to be defined: you likely forgot to import, pass, or provide it.");', + '}', '' ].join('\n'), 'should serialize fragments, namespaces, members' diff --git a/test/esm-loader.js b/test/esm-loader.js index 0960864..da9c7f2 100644 --- a/test/esm-loader.js +++ b/test/esm-loader.js @@ -26,7 +26,7 @@ test('xdm (ESM loader)', async (t) => { if (exception.code === 'ERR_UNKNOWN_FILE_EXTENSION') { await fs.unlink(path.join(base, 'esm-loader.mdx')) throw new Error( - 'Please run Node with `--experimental-loader=./esm-loader.js` to test the ESM loader' + 'Please run Node with `--experimental-loader=./test/react-18-esm-loader.js` to test the ESM loader' ) }