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

chore(ssr): use subpath imports for estree validators #5102

Closed
wants to merge 5 commits into from
Closed
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
6 changes: 6 additions & 0 deletions eslint.config.mjs
Original file line number Diff line number Diff line change
@@ -127,6 +127,12 @@ export default tseslint.config(
message:
'Do not directly import runtime flags from @lwc/features. Use the global lwcRuntimeFlags variable instead.',
},
{
name: 'estree-toolkit',
importNames: ['is'],
message:
'Do not import `is` from estree-toolkit directly. Import from #estree/validators instead.',
},
],
'no-restricted-properties': [
'error',
3 changes: 3 additions & 0 deletions packages/@lwc/ssr-compiler/package.json
Original file line number Diff line number Diff line change
@@ -47,6 +47,9 @@
}
}
},
"imports": {
"#estree/*": "./src/estree/*.js"
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I would avoid package imports/exports. We've had issues in the past with Rollup/Jest and how it consumes exports.

"dependencies": {
"@babel/types": "7.26.3",
"@lwc/shared": "8.12.3",
10 changes: 3 additions & 7 deletions packages/@lwc/ssr-compiler/src/__tests__/estemplate.spec.ts
Original file line number Diff line number Diff line change
@@ -5,15 +5,11 @@
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/

import { is, builders as b } from 'estree-toolkit';
import { builders as b } from 'estree-toolkit';
import { describe, test, expect } from 'vitest';
import { esTemplate, esTemplateWithYield } from '../estemplate';
import type { ClassDeclaration, FunctionDeclaration } from 'estree';

if (process.env.NODE_ENV !== 'production') {
// vitest seems to bypass the modifications we do in src/estree/validators.ts 🤷
(is.identifier as any).__debugName = 'identifier';
}
import { is } from '#estree/validators';

describe.each(
Object.entries({
@@ -61,7 +57,7 @@ describe.each(
`;
const doReplacement = () => tmpl(b.literal('I am not an identifier') as any);
expect(doReplacement).toThrow(
'Validation failed for templated node. Expected type identifier, but received Literal.'
/Validation failed for templated node. Expected type identifier, but received Literal./
);
});
});
Original file line number Diff line number Diff line change
@@ -6,13 +6,14 @@
*/

import { parse as pathParse } from 'node:path';
import { is, builders as b } from 'estree-toolkit';
import { esTemplate } from '../estemplate';
import { builders as b } from 'estree-toolkit';
import { bImportDeclaration } from '../estree/builders';
import { esTemplate } from '../estemplate';
import { bWireAdaptersPlumbing } from './wire';

import type { Program, Statement, IfStatement } from 'estree';
import type { ComponentMetaState } from './types';
import { is } from '#estree/validators';

const bGenerateMarkup = esTemplate`
const publicFields = new Set(${/*public fields*/ is.arrayExpression});
3 changes: 2 additions & 1 deletion packages/@lwc/ssr-compiler/src/compile-js/index.ts
Original file line number Diff line number Diff line change
@@ -6,7 +6,7 @@
*/

import { generate } from 'astring';
import { traverse, builders as b, is } from 'estree-toolkit';
import { traverse, builders as b } from 'estree-toolkit';
import { parseModule } from 'meriyah';

import { DecoratorErrors } from '@lwc/errors';
@@ -28,6 +28,7 @@ import type {
} from 'estree';
import type { Visitors, ComponentMetaState } from './types';
import type { CompilationMode } from '@lwc/shared';
import { is } from '#estree/validators';

const visitors: Visitors = {
$: { scope: true },
Original file line number Diff line number Diff line change
@@ -5,11 +5,11 @@
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/

import { is } from 'estree-toolkit';
import { generateScopeTokens } from '@lwc/template-compiler';
import { builders as b } from 'estree-toolkit/dist/builders';
import { builders as b } from 'estree-toolkit';
import { esTemplate } from '../estemplate';
import type { ExpressionStatement, Program, VariableDeclaration } from 'estree';
import { is } from '#estree/validators';

const bStylesheetTokenDeclaration = esTemplate`
const stylesheetScopeToken = '${is.literal}';
3 changes: 2 additions & 1 deletion packages/@lwc/ssr-compiler/src/compile-js/wire.ts
Original file line number Diff line number Diff line change
@@ -5,7 +5,7 @@
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/

import { is, builders as b } from 'estree-toolkit';
import { builders as b } from 'estree-toolkit';
import { produce } from 'immer';
import { DecoratorErrors } from '@lwc/errors';
import { esTemplate } from '../estemplate';
@@ -24,6 +24,7 @@ import type {
BlockStatement,
} from 'estree';
import type { ComponentMetaState, WireAdapter } from './types';
import { is } from '#estree/validators';

interface NoSpreadObjectExpression extends Omit<ObjectExpression, 'properties'> {
properties: Property[];
Original file line number Diff line number Diff line change
@@ -5,7 +5,6 @@
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/
import { builders as b } from 'estree-toolkit/dist/builders';
import { is } from 'estree-toolkit';
import { esTemplate, esTemplateWithYield } from '../estemplate';
import { isLiteral } from './shared';
import { expressionIrToEs } from './expression';
@@ -16,6 +15,7 @@ import type {
} from 'estree';
import type { TransformerContext } from './types';
import type { Node as IrNode, Text as IrText, Comment as IrComment } from '@lwc/template-compiler';
import { is } from '#estree/validators';

const bNormalizeTextContent = esTemplate`
normalizeTextContent(${/* string value */ is.expression});
3 changes: 2 additions & 1 deletion packages/@lwc/ssr-compiler/src/compile-template/index.ts
Original file line number Diff line number Diff line change
@@ -6,7 +6,7 @@
*/

import { generate } from 'astring';
import { is, builders as b } from 'estree-toolkit';
import { builders as b } from 'estree-toolkit';
import { parse, type Config as TemplateCompilerConfig } from '@lwc/template-compiler';
import { DiagnosticLevel } from '@lwc/errors';
import { esTemplate } from '../estemplate';
@@ -17,6 +17,7 @@ import { optimizeAdjacentYieldStmts } from './shared';
import { templateIrToEsTree } from './ir-to-es';
import type { ExportDefaultDeclaration as EsExportDefaultDeclaration } from 'estree';
import type { CompilationMode } from '@lwc/shared';
import { is } from '#estree/validators';

// TODO [#4663]: Render mode mismatch between template and compiler should throw.
const bExportTemplate = esTemplate`
4 changes: 2 additions & 2 deletions packages/@lwc/ssr-compiler/src/compile-template/ir-to-es.ts
Original file line number Diff line number Diff line change
@@ -7,7 +7,7 @@

import { inspect } from 'util';

import { is, builders as b } from 'estree-toolkit';
import { builders as b } from 'estree-toolkit';
import { esTemplate } from '../estemplate';
import { Comment } from './transformers/comment';
import { Component, LwcComponent } from './transformers/component';
@@ -26,7 +26,7 @@ import type {
} from '@lwc/template-compiler';
import type { Statement as EsStatement, ThrowStatement as EsThrowStatement } from 'estree';
import type { TemplateOpts, Transformer, TransformerContext } from './types';

import { is } from '#estree/validators';
const bThrowError = esTemplate`
throw new Error(${is.literal});
`<EsThrowStatement>;
3 changes: 2 additions & 1 deletion packages/@lwc/ssr-compiler/src/compile-template/shared.ts
Original file line number Diff line number Diff line change
@@ -5,7 +5,7 @@
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/

import { builders as b, is } from 'estree-toolkit';
import { builders as b } from 'estree-toolkit';
import { normalizeStyleAttributeValue, StringReplace, StringTrim } from '@lwc/shared';
import { isValidES3Identifier } from '@babel/types';
import { expressionIrToEs } from './expression';
@@ -28,6 +28,7 @@ import type {
Expression as IrExpression,
Literal as IrLiteral,
} from '@lwc/template-compiler';
import { is } from '#estree/validators';

export function optimizeAdjacentYieldStmts(statements: EsStatement[]): EsStatement[] {
let prevStmt: EsStatement | null = null;
Original file line number Diff line number Diff line change
@@ -5,7 +5,8 @@
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/

import { builders as b, is } from 'estree-toolkit';
import { builders as b } from 'estree-toolkit';

import { kebabcaseToCamelcase, toPropertyName } from '@lwc/template-compiler';
import { getChildAttrsOrProps } from '../../shared';
import { esTemplateWithYield } from '../../../estemplate';
@@ -14,6 +15,7 @@ import { getSlottedContent } from './slotted-content';
import type { BlockStatement as EsBlockStatement } from 'estree';
import type { Component as IrComponent } from '@lwc/template-compiler';
import type { Transformer } from '../../types';
import { is } from '#estree/validators';

const bYieldFromChildGenerator = esTemplateWithYield`
{
Original file line number Diff line number Diff line change
@@ -4,7 +4,6 @@
* SPDX-License-Identifier: MIT
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/
import { is } from 'estree-toolkit';
import { isUndefined } from '@lwc/shared';
import { expressionIrToEs } from '../../expression';
import { esTemplateWithYield } from '../../../estemplate';
@@ -16,6 +15,7 @@ import type {
Expression as IrExpression,
} from '@lwc/template-compiler';
import type { Statement as EsStatement } from 'estree';
import { is } from '#estree/validators';

const bYieldFromDynamicComponentConstructorGenerator = esTemplateWithYield`
const Ctor = '${/* lwcIs attribute value */ is.expression}';
Original file line number Diff line number Diff line change
@@ -6,7 +6,7 @@
*/

import { produce } from 'immer';
import { builders as b, is } from 'estree-toolkit';
import { builders as b } from 'estree-toolkit';
import { bAttributeValue, optimizeAdjacentYieldStmts } from '../../shared';
import { esTemplate, esTemplateWithYield } from '../../../estemplate';
import { irChildrenToEs, irToEs } from '../../ir-to-es';
@@ -35,6 +35,7 @@ import type {
Slot as IrSlot,
} from '@lwc/template-compiler';
import type { TransformerContext } from '../../types';
import { is } from '#estree/validators';

const bGenerateSlottedContent = esTemplateWithYield`
const shadowSlottedContent = ${/* hasShadowSlottedContent */ is.literal}
Original file line number Diff line number Diff line change
@@ -5,7 +5,7 @@
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/

import { builders as b, is } from 'estree-toolkit';
import { builders as b } from 'estree-toolkit';
import {
HTML_NAMESPACE,
isBooleanAttribute,
@@ -36,6 +36,7 @@ import type {
IfStatement as EsIfStatement,
} from 'estree';
import type { Transformer, TransformerContext } from '../types';
import { is } from '#estree/validators';

const bYield = (expr: EsExpression) => b.expressionStatement(b.yieldExpression(expr));

Original file line number Diff line number Diff line change
@@ -5,14 +5,15 @@
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/

import { builders as b, is } from 'estree-toolkit';
import { builders as b } from 'estree-toolkit';
import { esTemplate } from '../../estemplate';
import { irChildrenToEs } from '../ir-to-es';
import { getScopedExpression, optimizeAdjacentYieldStmts } from '../shared';

import type { ForEach as IrForEach } from '@lwc/template-compiler';
import type { Expression as EsExpression, ForOfStatement as EsForOfStatement } from 'estree';
import type { Transformer } from '../types';
import { is } from '#estree/validators';

const bForOfYieldFrom = esTemplate`
for (let [${is.identifier}, ${is.identifier}] of Object.entries(${is.expression} ?? {})) {
Original file line number Diff line number Diff line change
@@ -5,7 +5,7 @@
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/

import { builders as b, is } from 'estree-toolkit';
import { builders as b } from 'estree-toolkit';
import { esTemplate } from '../../estemplate';
import { irChildrenToEs } from '../ir-to-es';
import { optimizeAdjacentYieldStmts } from '../shared';
@@ -18,6 +18,7 @@ import type {
MemberExpression as EsMemberExpression,
} from 'estree';
import type { Transformer } from '../types';
import { is } from '#estree/validators';

function getRootMemberExpression(node: EsMemberExpression): EsMemberExpression {
return node.object.type === 'MemberExpression' ? getRootMemberExpression(node.object) : node;
Original file line number Diff line number Diff line change
@@ -5,7 +5,7 @@
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/

import { is, builders as b } from 'estree-toolkit';
import { builders as b } from 'estree-toolkit';

import { esTemplateWithYield } from '../../estemplate';

@@ -20,6 +20,7 @@ import type {
Expression as EsExpression,
} from 'estree';
import type { Transformer } from '../types';
import { is } from '#estree/validators';

const bConditionalSlot = esTemplateWithYield`
if (isLightDom) {
19 changes: 10 additions & 9 deletions packages/@lwc/ssr-compiler/src/estemplate.ts
Original file line number Diff line number Diff line change
@@ -16,15 +16,11 @@ import type {
Statement as EsStatement,
} from 'estree';
import type { Checker } from 'estree-toolkit/dist/generated/is-type';
import type { Validator } from './estree/validators';

/** Placeholder value to use to opt out of validation. */
const NO_VALIDATION = false;

/** A function that accepts a node and checks that it is a particular type of node. */
type Validator<T extends EsNode | null = EsNode | null> = (
node: EsNode | null | undefined
) => node is T;

/**
* A pointer to a previous value in the template literal, indicating that the value should be re-used.
* @see {@linkcode esTemplate}
@@ -93,15 +89,20 @@ const getReplacementNode = (
: validateReplacement(replacementNode))
) {
const expectedType =
(validateReplacement as any).__debugName ||
validateReplacement.name ||
'(could not determine)';
validateReplacement.__debugName || validateReplacement.name || '(could not determine)';
const actualType = Array.isArray(replacementNode)
? `[${replacementNode.map((n) => n && n.type).join(', ')}]`
: replacementNode?.type;
throw new Error(
const error = new Error(
`Validation failed for templated node. Expected type ${expectedType}, but received ${actualType}.`
);

if (validateReplacement?.__stack) {
error.message += `\n${validateReplacement.__stack.split('\n')[1]}`;
error.stack = validateReplacement.__stack;
}

throw error;
Comment on lines -96 to +105
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the type information, there's no need for as any.

}

return replacementNode;
30 changes: 26 additions & 4 deletions packages/@lwc/ssr-compiler/src/estree/validators.ts
Original file line number Diff line number Diff line change
@@ -5,11 +5,13 @@
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/

import { is } from 'estree-toolkit';
/* eslint-disable-next-line no-restricted-imports -- This is the only place where we should be importing `is` from estree-toolkit. */
import { is as _is } from 'estree-toolkit';

import { entries } from '@lwc/shared';
import type { Checker } from 'estree-toolkit/dist/generated/is-type';
import type { Node } from 'estree-toolkit/dist/helpers'; // estree's `Node` is not compatible?

import type { Node as EsNode } from 'estree';
/** A validator that returns `true` if the node is `null`. */
type NullableChecker<T extends Node> = (node: Node | null | undefined) => node is T | null;

@@ -26,9 +28,29 @@ export function isNullableOf<T extends Node>(validator: Checker<T>): NullableChe

isNullableOf.__debugName = 'isNullableOf';

/** A function that accepts a node and checks that it is a particular type of node. */
export type Validator<T extends EsNode | null = EsNode | null> = {
(node: EsNode | null | undefined): node is T;
__debugName?: string;
__stack?: string;
};

const is: {
[K in keyof typeof _is]: (typeof _is)[K] & { __debugName?: string; __stack?: string };
} = { ..._is };

if (process.env.NODE_ENV !== 'production') {
// Modifying another package's exports is a code smell!
for (const [key, val] of entries(is)) {
(val as any).__debugName = key;
val.__debugName = key;
Copy link
Collaborator

Choose a reason for hiding this comment

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

val here is still the function from estree-toolkit, so is.identifier imported via the actual package will still be modified. 😓

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. I guess the only immediate win is making this compatible with esm, which admittedly isn't much for a debug-only feature.

Object.defineProperty(is, key, {
get: function get() {
const stack: { stack?: string } = {};
Error.captureStackTrace(stack, get);
val.__stack = stack.stack;
return val;
},
});
Comment on lines +44 to +52
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This feature, if deemed useful, can be added in a separate PR.

}
}

export { is };
3 changes: 2 additions & 1 deletion packages/@lwc/ssr-compiler/tsconfig.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
{
"extends": "../../../tsconfig.json",
"compilerOptions": {
"types": ["rollup", "node"]
"types": ["rollup", "node"],
"rootDir": "src"
},
"include": ["src/"]
}
5 changes: 4 additions & 1 deletion packages/@lwc/template-compiler/src/parser/html.ts
Original file line number Diff line number Diff line change
@@ -36,7 +36,10 @@ function getLwcErrorFromParse5Error(ctx: ParserCtx, code: string) {
}
}

export function parseHTML(ctx: ParserCtx, source: string) {
export function parseHTML(
ctx: ParserCtx,
source: string
): parse5.DefaultTreeAdapterTypes.DocumentFragment {
Comment on lines -39 to +42
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes an error/warning caught after the tsconfig changes:

The inferred type of 'parseHTML' cannot be named without a reference to '../../../../../node_modules/parse5/dist/tree-adapters/default'. This is likely not portable. A type annotation is necessary.ts(2742)

const onParseError = (err: parse5.ParserError) => {
const { code, ...location } = err;

4 changes: 3 additions & 1 deletion scripts/tasks/check-and-rewrite-package-json.js
Original file line number Diff line number Diff line change
@@ -47,7 +47,8 @@ for (const dir of globSync('./packages/@lwc/*')) {
continue;
}

const { name, description, version, dependencies, devDependencies, peerDependencies } = pkg;
const { name, description, version, dependencies, devDependencies, peerDependencies, imports } =
pkg;
let { keywords } = pkg;

// Keywords aren't really important, but keep any that already exist and add 'lwc'
@@ -112,6 +113,7 @@ for (const dir of globSync('./packages/@lwc/*')) {
extends: '../../../package.json',
},
...buildProps,
imports,
dependencies,
devDependencies,
peerDependencies,
5 changes: 3 additions & 2 deletions tsconfig.json
Original file line number Diff line number Diff line change
@@ -6,8 +6,9 @@
"declarationMap": true,
"sourceMap": true,

"module": "commonjs",
"moduleResolution": "node",
"module": "ESNext",
"moduleResolution": "bundler",
"resolvePackageJsonImports": true,
"esModuleInterop": true,

"target": "es2021",