Fix values and types merging in JS module exports#37896
Fix values and types merging in JS module exports#37896andrewbranch merged 6 commits intomicrosoft:masterfrom
Conversation
sandersn
left a comment
sandersn
left a comment
There was a problem hiding this comment.
This seems like the right approach for a fix. I think the real, underlying problem is that module.exports doesn't create real aliases, but that's much more work to fix comprehensively.
I'd follow this up with a user test run.
tests/cases/compiler/jsExportMemberMergedWithModuleAugmentation.ts
Outdated
Show resolved
Hide resolved
|
@typescript-bot user test this |
|
Heya @andrewbranch, I've started to run the parallelized community code test suite on this PR at c6f7c82. You can monitor the build here. |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
sandersn
left a comment
sandersn
left a comment
There was a problem hiding this comment.
The basic idea is good, just a couple of things I'd like to know about.
src/compiler/checker.ts
Outdated
| isEffectiveModuleDeclaration(target.valueDeclaration) && !isEffectiveModuleDeclaration(source.valueDeclaration))) { | ||
| !(source.valueDeclaration.flags & NodeFlags.Ambient && !(target.valueDeclaration?.flags & NodeFlags.Ambient)) && | ||
| (isAssignmentDeclaration(target.valueDeclaration) && !isAssignmentDeclaration(source.valueDeclaration) || | ||
| isEffectiveModuleDeclaration(target.valueDeclaration) && !isEffectiveModuleDeclaration(source.valueDeclaration)))) { |
There was a problem hiding this comment.
does this need a fix in the binder too? It probably only applies to code that is js-only (assignment declarations) and ts-only (ambient declarations) so 99.9% of the time it'll be in different files, but for the 0.01%, we do understand ambient declarations in .js as well -- we just put an error on them.
There was a problem hiding this comment.
Hmm, how would you end up with an ambient declaration in a JavaScript file? Off the top of my head I didn’t think that was possible.
There was a problem hiding this comment.
Well, we'll parse and understand this in JS:
declare function f(): void
f()Just with lots of errors saying "please don't use this".
More practically, I think it would be better if this code and the check inside declareSymbol in the binder were the same -- if they haven't already diverged too far.
There was a problem hiding this comment.
The logic in those two places was identical, so it was safe and easy to move to a utility. That said, I couldn’t actually trigger the same crash by writing ambient code in a JS file, I think because an ambient module declaration is only a module augmentation if the file is an ES module, and if it’s an ES module, module.exports is ignored.
|
Since I'm out tomorrow and the beta is done tomorrow, go ahead and merge this if there's no easy reconciliation between the module priority code in mergeSymbol and declareSymbol. We probably should dedupe the code, but if it's anything but trivial, it's a risky change. |
3 participants
I don’t think this is exactly right, but it’s probably closer than what was there before. Opening to create a context to chat about this with @sandersn.
Fixes #37833