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

cardoso · 2025-01-07T15:46:22Z

Details

Currently, the is import from estree-toolkit is conditionally modified to add some debug information:

lwc/packages/@lwc/ssr-compiler/src/estree/validators.ts

Lines 29 to 34 in b2468c9

    
           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; 
        
               } 
        
           }

However this only works for cjs (require), not esm (import), which is explains the setup code in this test:

lwc/packages/@lwc/ssr-compiler/src/__tests__/estemplate.spec.ts

Lines 13 to 16 in b2468c9

    
           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'; 
        
           }

This PR aims to replace this approach by exporting a new is object from estree/validators.ts instead, including types for the debug info.

All other files previously importing is from estree-toolkit are modified to import is from estree/validators.ts.

To make this easier a subpath import was added: #estree/* so the import can easily be changed from estree-toolkit to #estree/validators.

This required changing a few compiler options in tsconfig.json (previously discussed here: #4484 (comment)).

TODO

Confirm changes in tsconfig.json and subpath imports won't cause breaking changes

Does this pull request introduce a breaking change?

😮‍💨 No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

🤞 No, it does not introduce an observable change.

GUS work item

cardoso

I added a __stack feature for the validators which might be useful, but it might be better to make a follow up PR for it. It works but I didn't give it much thought.

Error: Validation failed for templated node. Expected type identifier, but received Literal.
    at /lwc/packages/@lwc/ssr-compiler/src/compile-template/transformers/text.ts:24:69
 ❯ ../ssr-compiler/src/compile-template/transformers/text.ts:24:69
     22| const bBufferTextContent = esTemplateWithYield`
     23|     didBufferTextContent = true;
     24|     textContentBuffer += massageTextContent(${/* string value */ is.identifier});
       |                                                                     ^
     25| `<EsExpressionStatement[]>;
     26| 
 ❯ ../ssr-compiler/src/compile-template/ir-to-es.ts:11:32

cardoso · 2025-01-07T15:48:26Z

packages/@lwc/ssr-compiler/src/estemplate.ts

-            (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;


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

cardoso · 2025-01-07T15:50:06Z

packages/@lwc/ssr-compiler/src/estree/validators.ts

+        val.__debugName = key;
+        Object.defineProperty(is, key, {
+            get: function get() {
+                const stack: { stack?: string } = {};
+                Error.captureStackTrace(stack, get);
+                val.__stack = stack.stack;
+                return val;
+            },
+        });


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

cardoso · 2025-01-07T15:54:12Z

packages/@lwc/template-compiler/src/parser/html.ts

-export function parseHTML(ctx: ParserCtx, source: string) {
+export function parseHTML(
+    ctx: ParserCtx,
+    source: string
+): parse5.DefaultTreeAdapterTypes.DocumentFragment {


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)

wjhsf

__debugName was meant to be a temporary hack because we were doing a lot of changes to the templates and the errors weren't great for debugging. This feels like making it a permanent hack, but I'm not convinced that's something we want to do.

packages/@lwc/ssr-compiler/src/__tests__/estemplate.spec.ts

cardoso · 2025-01-07T16:18:05Z

@wjhsf do you have a better idea for __debugName? The primary motivation for this PR is to avoid modifying estree-toolkit directly. Also to move away from the node (aka node10) module resolution (#4484 (comment)).

Co-authored-by: Will Harney <[email protected]>

…e-validators

wjhsf · 2025-01-08T19:58:52Z

packages/@lwc/ssr-compiler/src/estree/validators.ts

    for (const [key, val] of entries(is)) {
-        (val as any).__debugName = key;
+        val.__debugName = key;


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

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

nolanlawson · 2025-01-08T20:00:08Z

packages/@lwc/ssr-compiler/package.json

@@ -47,6 +47,9 @@
            }
        }
    },
+    "imports": {
+        "#estree/*": "./src/estree/*.js"
+    },


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

nolanlawson · 2025-01-09T22:09:05Z

Thanks for the PR! Overall this feels like a lot of added code and complexity for what is essentially just nicer dev error messages... A couple questions:

Can we use a WeakMap or similar to avoid monkey-patching in the first place?
What about upstreaming something into estree-toolkit itself? Is there some way we can improve the debuggability there instead?

cardoso · 2025-01-10T11:33:40Z

@nolanlawson @wjhsf thanks for the feedback! Let me rethink this one.

I'll see if something can be done in estree-toolkit but I think its validators just weren't designed to be used in this way.

Also how often is this debug info being useful for you? Isn't typescript catching everything? Considering that, would the __stack feature experimented with here be relevant?

wjhsf · 2025-01-10T15:22:36Z

Also how often is this debug info being useful for you?

Not very often, we went with the quick ugly hack because we were planning on removing it once ssr-compiler is stable.

Isn't typescript catching everything?

Not quite, because we have to manually introduce types when we use esTemplate, and those might not always be accurate if we forget to keep them updated.

Considering that, would the __stack feature experimented with here be relevant?

It's neat! But we usually already have a pretty good idea of which thing is causing an error, because we're usually only screwing up one esTemplate at a time. Maybe it'd be worth including if it's a small change on its own? Or maybe we're close enough to done now that it's not worth the effort. idk 🤷

cardoso added 2 commits January 7, 2025 11:57

chore(ssr): use subpath imports for validators

60093f1

test: validator reports location

f1b3047

cardoso requested a review from a team as a code owner January 7, 2025 15:46

salesforce-cla bot added the cla:signed label Jan 7, 2025

cardoso commented Jan 7, 2025

View reviewed changes

wjhsf reviewed Jan 7, 2025

View reviewed changes

packages/@lwc/ssr-compiler/src/__tests__/estemplate.spec.ts Outdated Show resolved Hide resolved

cardoso and others added 3 commits January 7, 2025 13:23

Update packages/@lwc/ssr-compiler/src/__tests__/estemplate.spec.ts

2c92781

Co-authored-by: Will Harney <[email protected]>

Merge branch 'master' of https://github.com/salesforce/lwc into estre…

ef101cb

…e-validators

Merge branch 'master' of https://github.com/salesforce/lwc into estre…

6ac97d1

…e-validators

wjhsf reviewed Jan 8, 2025

View reviewed changes

nolanlawson reviewed Jan 8, 2025

View reviewed changes

cardoso closed this Jan 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cardoso commented Jan 7, 2025

cardoso left a comment •

edited

Loading

cardoso Jan 7, 2025

cardoso Jan 7, 2025

cardoso Jan 7, 2025

wjhsf left a comment

cardoso commented Jan 7, 2025

wjhsf Jan 8, 2025

cardoso Jan 8, 2025

nolanlawson Jan 8, 2025

nolanlawson commented Jan 9, 2025

cardoso commented Jan 10, 2025

wjhsf commented Jan 10, 2025

	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;
	}
	}

	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';
	}

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

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

Conversation

cardoso commented Jan 7, 2025

Details

TODO

Does this pull request introduce a breaking change?

Does this pull request introduce an observable change?

GUS work item

cardoso left a comment • edited Loading

Choose a reason for hiding this comment

cardoso Jan 7, 2025

Choose a reason for hiding this comment

cardoso Jan 7, 2025

Choose a reason for hiding this comment

cardoso Jan 7, 2025

Choose a reason for hiding this comment

wjhsf left a comment

Choose a reason for hiding this comment

cardoso commented Jan 7, 2025

wjhsf Jan 8, 2025

Choose a reason for hiding this comment

cardoso Jan 8, 2025

Choose a reason for hiding this comment

nolanlawson Jan 8, 2025

Choose a reason for hiding this comment

nolanlawson commented Jan 9, 2025

cardoso commented Jan 10, 2025

wjhsf commented Jan 10, 2025

cardoso left a comment •

edited

Loading