-
Couldn't load subscription status.
- Fork 724
Actually transform KindCommonJSExport in declaration emit #1962
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a crash in declaration emit caused by missing handling for the KindCommonJSExport node type, which was introduced during earlier CommonJS module parsing work but never implemented in the declaration transformer.
Key Changes
- Added declaration emit support for
KindCommonJSExportnodes - Handles both identifier-named exports (including
default) and string-named exports with appropriate transformations - Added test case to prevent regression
Reviewed Changes
Copilot reviewed 68 out of 68 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
testdata/tests/cases/compiler/jsDeclarationExportDefaultAssignmentCrash.ts |
New test case for CommonJS default export crash |
internal/transformers/declarations/util.go |
Added KindCommonJSExport to nodes with inferred types |
internal/transformers/declarations/transform.go |
Implemented declaration emit transformation for CommonJS exports |
| Multiple baseline files | Updated test baselines showing improved declaration emit output for CommonJS patterns |
| @@ -30,7 +30,7 @@ | |||
| //// [a.d.ts] | |||
| -export const x: 0; | |||
| +export declare const x = 0; | |||
| +export var y = 0; | |||
| +export declare var y: number; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw this when trying to fix this crash too; definitely some weird widening interplay here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the old output was because the KindCommonJSExport declaration was just being emitted as-is. :S That's why we had things like export var x = { x: "x" } in the output which just...isn't actually something we should ever have in a declaration file? And is an error?
| +export declare var x: number; | ||
| +export declare var y: undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird, we're taking the initial assignment for the type of these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ensureType uses the serialized type of the declaration which, generally speaking, is inferred from the first initializer. It's just whatever getTypeOfSymbol returns, without any special handling for const-y-ness, excluding some special cases it already handles for TS code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving though because as you say, you know about the ick things, so I'd rather have the crash fixed and it be less ick unless you see an obvious way to fix the bigger icks.
Probably worth a new issue about it generally.
|
Agh bitten by leftover baselines (main shouldn't do this anymore, usually) |
JS declaration emit, in general, needs a polish/completeness pass - nobody's just gone through the js declaration baselines and fixed all the diffs yet, since we've still been working on JS handing at the reparser step, afaik. What works is mostly by chance and commonality with TS (which is perhaps unsurprisingly a lot of it!). |
Fixes #1178
The bug was less that something was broken, and more like nobody ever added handling for the newly reparsed node kind to declaration emit at all. I'm not 100% sure how these should be translated to declarations, but I think I have reasonable transforms in the code. This is enough implementation to fix the crashes, but, looking at the baselines, behavior in some cases stands to be improved (since this is mostly just variants of the export assignment handling logic), as it produces some pretty ick output. In particular, multiple declarations not merging is... OK (different from strada, ofc), but maybe could use some checks to see if they can combine/be elided to improve the output. Additionally, some namespacey patterns that are seemingly not well supported checking-ways anymore have seemingly equally poor declaration emit (eg,
var ns: typeof ns). Additionally,const-ness and maybe-const-initializer-y-ness of the variables isn't correctly handled yet (instead the non-const type is always emitted), which requires integrating some of theexport varlogic into the export assignment logic I reused. IMO, those are all reasonable follow-up improvements since I'm still trying to focus on normal emit, and really just wanted to fix the crash here.