module.exports = Entity is an alias, just like export = Entity

[sandersn]

module.exports = Entity should create an alias the same way that export = Entity does in typescript. Doing this allows the type of a class to resolve correctly, for example.

The PR has a couple of other changes.

1. getTypeFromImportTypeNode needs a special case for import types in JSDoc to avoid the "Module 'foo' does not refer to a type but is used as a type here" error.
2. The recently added code to allow both module.export= assignments and module.export.property= assignments in JS now needs to resolve aliases first. I also made that code use mergeSymbolTable since it was basically identical.
3. Alias resolution needs to understand module.exports= declarations. This is a small change to getTargetOfExportAssignment and its surroundings.

Fixes #23375

[weswigham]

When I ran your new test with the debugger, this clause wasn't executed, so it seems unnecessary (indeed, I can't figure out when you'd need it, with the binder fixed up); what prompted it? (Indeed - I deleted it and no tests failed, leading me to believe this may be a bit vestigial from before you fixed the binder)

[sandersn]

I guess I am just crazy, because I saw failures this morning when trying to get rid of this.

Except...I didn't just try to get rid of it, I tried to refactor it earlier in the function. So I guess it is indeed vestigial from my work yesterday.

[weswigham]

So this is subtly different - if you somehow bind an export= and normal exports, this will merge the normal export with an export= member of the same name (creating a merge symbol in the process), whereas previously the first symbol in won.

[weswigham]

Which, I guess in JS might actually be possible, since you can do

module.exports.x = 2;
class Foo {};
module.exports = Foo; // should hide all prior assignments

[sandersn]

It is the exported thing that has to have a property named export= for this to manifest, not the module. In what cases can the exported thing have export=? I can't think of any. Namespaces don't exist in JS, and that's the only construct I am not sure about.

(Your example is a real problem, it's just not related to this change.)

[sandersn]

After much discussion and experimentation, it looks like I can move this merging code (added in #23228) into the binder as long as we correctly handle conflicts between property assignments names and contents of the export assignment (eg module.exports.x = ... and module.exports = { x: ... }.

However, that change is not trivial, so I want to put it in a separate PR so it doesn't block webpack PR webpack/webpack#7031 that is waiting on this PR.

[sandersn]

However, I think I will revert it since it will be going away in the next few days anyway.

[sandersn]

Note that this code still doesn't work for class expressions (from Webpack):

// in WebpackError.js
module.exports = class WebpackError extends Error {
	constructor(message) {
		super(message);

		this.details = undefined;

		Error.captureStackTrace(this, this.constructor);
	}

	inspect() {
		return this.stack + (this.details ? `\n${this.details}` : "");
	}
};
// in Dependency.js
class Dependency {
	/** @return {null | (import("./WebpackError"))[]} */
	getWarnings() {
		return null;
	}
}

But it now works when exporting an alias:

// in WebpackError.js
class WebpackError extends Error {
	constructor(message) {
		super(message);

		this.details = undefined;

		Error.captureStackTrace(this, this.constructor);
	}

	inspect() {
		return this.stack + (this.details ? `\n${this.details}` : "");
	}
};
module.exports = WebpackError

[sandersn]

Typescript behaves exactly the same way: 😿

// in first.ts
export = class Huh {
    x = 1
}
// in main.ts
import Huh = require('./first')
declare var huh: Huh // should not error on type name Huh, but does

[weswigham]

@sandersn Which doesn't work mostly for the same reasons export default (class Foo {}); doesn't in TS - because we just bind the expression as a valuespace expression without inspecting it, since we've checked that it's not an entity name. Should we add another special case where we allow class expressions in export assignments to create type meanings? (Maybe just in Js?)

[sandersn]

Yes, probably.

It should work in TS too, unless there are weird problems with declaration emit. I don't think there are, but can you think of any?

[weswigham]

@sandersn erm.... Technically I don't think we parse declare modifiers on class expressions. I mean, the new declaration emitter can manage some way to print it that'll roundtrip, but I don't think just reemiting export = declare class {} will necessarily work (unless we do parse modifiers there, in which case... Maybe?). In any case, there's some way to rewrite it (nonexported class that gets alised, perhaps) in the new declaration emitter, for sure.

[sandersn]

OK, exported class expressions can now be used as types when imported. This works in both JS and TS.

[weswigham]

Just the one comment I made in person about how unnamed classes aught to be fine (maybe wanna test that), but otherwise looks good.

[sandersn]

Yep, works fine without a name. I totally forgot that importing the class would give it a name.