Fix error on duplicate commonjs exports #40545
Conversation
Previously, the code missed setting the parent pointer for the lhs access expression. Also add declaration emit of element access expressions, missed in my previous PR.
typescript-bot
added
Author: Team
For Uncommitted Bug
DanielRosenwasser
added
the
Breaking Change
|
|
||
| //// [moduleExportAliasElementAccessExpression.js] | ||
| function D() { } | ||
| exports["D"] = D; |
There was a problem hiding this comment.
And non-identifier names, like "A B", emit as...what? Probably needs a test.
There was a problem hiding this comment.
_A_B. I added a test the in the bug, but I guess I can add it here.
| ~~~~~~~~~~~~~ | ||
| !!! error TS2322: Type '() => void' is not assignable to type 'undefined'. | ||
| ~~~~~ | ||
| !!! error TS2300: Duplicate identifier 'apply'. |
There was a problem hiding this comment.
To get rid of this error, we can defer detection of this case in the binder by removing the offending case from AliasExcludes (like we did for class/function merges) and then look for it in the checker, where we can filter certain declarations (probably undefined assignments) and except it, while retaining the error more generally.
I think this pattern is actually super common, considering it's our current emit for cjs, so we should probably invest the effort into making it work smoothly.
There was a problem hiding this comment.
Per our discussion, I switched this to None for the beta. It turns out that the checker already issues an error in this case, but we can add an exclusion for undefined-as-initialiser later.
CommonJS exports have None excludes, but still have an error issued by the checker. This is the previous behaviour even though it would be nice to add some exclusions.
There was a problem hiding this comment.
I guess this is ok, so long as we're tracking fixing the error more broadly and adding an exception to the checker's duplicate export checking for undefined assignments (because our own emit looks like that). At least for the former, as I mentioned, I am still sliiiiiiightly worried that not using the appropriate exclude flags might result in some order-dependent cross-file merge issues down the line; so fixing it by adjusting the exclude flags themselves may be pertinent in the near future.
typescript-bot
added
For Milestone Bug
|
Filed #40555 to follow up. |
DanielRosenwasser
removed
the
Breaking Change
Previously, the code missed setting the parent pointer for the lhs access expression.
Note that this was not previously an error; you can also avoid the error by setting the exclude flags back to
None. But I don't think duplicate exports are that common, and non-checkjs users aren't going to see the error anyway.Also add declaration emit of element access expressions, missed in my previous PR.
Fixes #40513