-
Notifications
You must be signed in to change notification settings - Fork 707
fix(1685): refine export assignment diagnostics to match strada #1688
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
base: main
Are you sure you want to change the base?
Conversation
+ */ | ||
+ | ||
+ | ||
+//// [DtsFileErrors] |
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.
This one's a regression.
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.
@jakebailey This may not be directly related to the change; it seems the issue is that the jsdoc callback is emitted as exported in the .d.ts
files.
typescript-go/internal/parser/reparser.go
Line 73 in d2c442d
// !!! Don't mark typedefs as exported if they are not in a module |
export = MyClass;
....
export type DoneCB = (failures: number) ;
while strada
handles this case as
export = MyClass;
....
type DoneCB = (failures: number) => any;
I think the way to address this regression is to elide the export modifier. WDYT?
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.
That sounds plausible. Thankfully I believe ExternalModuleIndicator
is set before reparsing.
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.
It looks like ExternalModuleIndicator
is set only after parsing top-level statements
typescript-go/internal/parser/parser.go
Line 350 in c0aa663
p.finishSourceFile(result, isDeclarationFile) |
typescript-go/internal/parser/parser.go
Line 379 in c0aa663
ast.SetExternalModuleIndicator(result, p.opts.ExternalModuleIndicatorOptions) |
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.
Yeah, the declaration emitter or reparser definitely needs to be adjusted to handle this. Probably the reparser, in this case - it probably shouldn't add an export
modifier if there's an export assignment in the file. Truth be told, though, this is one of those cases where we can't accurately represent the input as a declaration file, and no matter what we do we're probably going to change behavior from strada.
@sandersn @weswigham Is this the thing we were discussing about relaxing this for even TS code and not erroring at all? I can't remember where we left that. |
We're at the "just do it like strada" stage because trying to improve it just moved where the jank in our code is - better the jank we know. |
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.
Definitely an improvement. I'm sure we'll have to keep adjusting the JS declaration emit for export assignments, though, since they're still only partially supported.
|
||
|
||
==== out/source.d.ts (2 errors) ==== | ||
export = MyClass; |
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.
Hm. This is because we don't actually support constructor functions yet, right @sandersn ?
-export type SomeType = { | ||
+type SomeType = { |
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.
This one is regressing; maybe a mad interplay with the new code?
-export type Conn = import("./conn"); | ||
+type Conn = import("./conn"); |
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.
This one too? (I hate JSDoc typedef rules)
-export type TaskGroupIds = "parseHTML" | "styleLayout"; | ||
+export type TaskGroupIds = 'parseHTML' | 'styleLayout'; | ||
export type TaskGroup = { | ||
-export type TaskGroup = { | ||
+type TaskGroupIds = 'parseHTML' | 'styleLayout'; | ||
+type TaskGroup = { |
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.
This file's also regressing in a similar way, it seems...
Fixes #1685
This patch addresses the issue of incomplete export assignment validation by aligning the check with
Strada
logic.