Port JSX Emit #1159
Port JSX Emit #1159
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR, titled "Port JSX Emit", addresses two main issues: ensuring that files containing JSX tags are correctly marked as modules to allow proper JSX emit, and implementing printer.EFStartOnNextLine so that tag emit occurs on separate lines. Key changes include updating baseline outputs for JSX expressions to use React.createElement, refactoring JSX intrinsic name checks to use the new scanner.IsIntrinsicJsxName function, and incorporating JSX transformers into the emitter workflow.
Reviewed Changes
Copilot reviewed 429 out of 429 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| testdata/baselines/reference/submodule/compiler/commentsOnJSXExpressionsArePreserved(jsx=react,module=commonjs,moduledetection=legacy).* | Updated conversion of JSX literals to React.createElement calls for legacy module detection |
| testdata/baselines/reference/submodule/compiler/commentsOnJSXExpressionsArePreserved(jsx=react,module=commonjs,moduledetection=force).* | Similar JSX output changes for force module detection |
| testdata/baselines/reference/submodule/compiler/commentsOnJSXExpressionsArePreserved(jsx=react,module=commonjs,moduledetection=auto).* | Alignment of JSX baseline outputs for auto module detection |
| testdata/baselines/reference/submodule/compiler/callsOnComplexSignatures.* | Adjusted component rendering syntax with new element creation patterns and const declarations |
| internal/transformers/, internal/printer/, internal/checker/, internal/scanner/, internal/ast/* | Refactored helper methods, updated intrinsic JSX name checks, and added new functionality for JSX emit and assign helpers |
Comments suppressed due to low confidence (2)
internal/checker/grammarchecks.go:1198
- Verify that scanner.IsIntrinsicJsxName fully replicates the previous behavior of IsIntrinsicJsxName to prevent any regression in handling JSX names for namespaced nodes.
if ast.IsJsxNamespacedName(node) && c.compilerOptions.GetJSXTransformEnabled() && !scanner.IsIntrinsicJsxName(node.AsJsxNamespacedName().Namespace.Text()) {
internal/checker/utilities.go:1336
- Ensure that transitioning to scanner.IsIntrinsicJsxName from the previous version does not introduce subtle differences in JSX tag name validation.
return ast.IsIdentifier(tagName) && scanner.IsIntrinsicJsxName(tagName.Text()) || ast.IsJsxNamespacedName(tagName)
This should now be there in main as of earlier today, so that should just work if you merge. |
Yes, but on investigation, a weeee bit off. One of those |
|
Great, thanks! 😄 |
| @@ -38,21 +38,19 @@ | |||
| // Pair of non-like intrinsics | |||
| function render(url) { | |||
| - var Tag = url ? 'a' : 'button'; | |||
There was a problem hiding this comment.
I'm working on dropping ES5 out of the submodule, so theoretically that'd eliminate a load of diffs here too.
| + | ||
| + | ||
| + </div>; | ||
| + return jsx_runtime_1.jsx("div", { children: null /* preserved */ }); |
There was a problem hiding this comment.
One difference I'm seeing is that the (0, ...) is missing. I'm not 100% sure who's in charge of adding that.
There was a problem hiding this comment.
No idea tbh - not the JSX transform, tho, that's for sure. I think it's a downleveling one, though.
| + | ||
| + | ||
| + </div>; | ||
| + return _jsx("div", { children: null /* preserved */ }); | ||
| } | ||
| - Component.prototype.render = function () { | ||
| - return (0, _a.jsx)("div", { children: null /* preserved */ }); |
There was a problem hiding this comment.
Seemingly missing whatever _a was?
There was a problem hiding this comment.
This is the emit when the file's module transform isn't the same as in Strada, as the substitution from _jsx to mod.jsx is done by the cjs transform. It implies something is still off with the module detection logic or substitution compared to strada - in this case, when moduleDetection: legacy is set. In this case, though, I don't think it's an important diff - the behavior of jsx auto-imports in script files (which this is) is explicitly not actually stable.
Yeah, the vast majority remaining are |
|
Happy to see someone working on the emit side! 🙏🏽 |
I'm still going through the baseline diffs and trying to minimize the bad ones, but there are two notable things I've seen already:
printer.EFStartOnNextLineis not yet implemented, so all tag emit occurs on one line (this is also used lightly by the cjs module transform)