Always export typedefs#23723
Conversation
This actually just required deleting a check in declareModuleMembers and checking for external AND commonjs modules in a couple of places. However, while experimenting with this feature, I discovered that even previously-exported typedefs would only be exported if they came after a commonjs export node. So I added a commonjs check to the pass in the parser. It will not catch nested module.exports, but it will catch top-level assignments. The new test tests both changes.
| } | ||
|
|
||
| function isCommonJsModuleIndicator(statement: Statement) { | ||
| if (isExpressionStatement(statement) && isBinaryExpression(statement.expression)) { |
There was a problem hiding this comment.
this is concerning.. first it is work that were not doing before, and second, it is potentially incomplete. in the binder we bind all require calls, not just the top-level ones, and not just binary expressions. so to do this right we need to walk the whole file until we find an indicator.. but we are doing this in the binder already..
is there a different way we can collect these typedefs then bind their export symbols at the end?
There was a problem hiding this comment.
That sounds like a good approach -- less lossy although less obvious. I'll see if I can get something working.
|
OK, typedefs are now bound at the end of source file binding, and I got rid of the check for commonjs constructions in the parser. |
weswigham
left a comment
There was a problem hiding this comment.
Do we have any tests of typedefs merging with the various export assignments when those assignments also have value-space members if the same name? Or errors when they conflict, eg,
/** @typedef {{a: string}} Foo */
exports.Foo = class Foo {}|
@weswigham here are some more tests: /** @typedef {number} Foo */
class Foo {} // error/** @typedef {number} Foo */
exports.Foo = class { } // error (and with module.exports too)/** @typedef {number} Foo */
module.exports = { Foo: class { } } // errorAll of the above should pass when Foo is |
|
@weswigham I got something that works ... kind of ... take a look and see. Let me know if you can think of anything better. I still need to add baselines and update existing ones. I'm not sure I like this solution, but I wanted to share it so others could look at it. |
|
@sandersn here's a few more that I think may(?) throw a wrench into your current solution /** @typedef {number} Foo */
const Ns = {};
Ns.Foo = class {}
module.exports = Ns;also /** @typedef {number} Foo */
class Bar {}
module.exports = { Foo: Bar };😄 |
|
I bet if I special cased object literal initializers in bindModuleExportAssignment so that all the properties in the object literal get added to the exports, that would do the trick, and prevent conflicts that I don’t think we catch today, like duplication between module.exports.p = 2 and module.exports = { p: 2 } |
That's less duplication and more reassignment (I think) - kinda like export let f = 2;
f = 3;in TS. |
|
Yep, after thinking about it I realized that it would break the special-case type unioning code in the checker, too, which we want to keep. |
| }); | ||
| if (symbol) { | ||
| declareSymbol(symbol.exports, symbol, lhs, SymbolFlags.Property | SymbolFlags.ExportValue, SymbolFlags.None); | ||
| const flags = isClassExpression(node.right) ? |
There was a problem hiding this comment.
not sure i understand why this is related ot the original change?
There was a problem hiding this comment.
Now that typedefs are exported, there's a possibility that their names collide, such that there is a difference between
let x: import("./a.js").Foo
// and
import a = require('./a.js')
var foo = new Foo()In other words, the type reference Foo might refer to the typedef, whereas the value referenced in new Foo might refer to the class. There should be an error to prevent this.
This actually just required deleting a check in declareModuleMembers and checking for external AND commonjs modules in a couple of places.
However, while experimenting with this feature, I discovered that even previously-exported typedefs would only be exported if they came after a commonjs export node. So I added a commonjs check to the pass in the parser. It will not catch nested module.exports, but it will catch top-level assignments.
The new test tests both changes.
Fixes #23692