`module.exports = Entity` is an alias, just like `export = Entity` by sandersn · Pull Request #23570 · microsoft/TypeScript

sandersn · 2018-04-20T16:34:24Z

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.

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.
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.
Alias resolution needs to understand module.exports= declarations. This is a small change to getTargetOfExportAssignment and its surroundings.

Fixes #23375

This breaks a couple of tests for previous workarounds. Fix in upcoming commits.

instead of basically copying the code myself.

weswigham · 2018-04-20T17:45:08Z

                    if (moduleSymbol.flags & targetMeaning) {
                        resolveImportSymbolType(node, links, moduleSymbol, targetMeaning);
                    }
+                    else if (node.flags & NodeFlags.JSDoc && moduleSymbol.flags & SymbolFlags.Value) {


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)

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 · 2018-04-20T17:47:42Z

+                merged.flags = merged.flags | SymbolFlags.ValueModule;
+                merged.exports = createSymbolTable();
+            }
+            mergeSymbolTable(merged.exports, moduleSymbol.exports);


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.

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

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.)

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.

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

It is more correct and will go away in a few days.

sandersn · 2018-04-23T17:17:39Z

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 · 2018-04-23T17:22:43Z

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 · 2018-04-23T17:22:47Z

@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 · 2018-04-23T17:24:54Z

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 · 2018-04-23T17:30:20Z

@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.

In both JS and TS

sandersn · 2018-04-23T21:43:08Z

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 · 2018-04-23T22:09:10Z

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

sandersn added 6 commits April 19, 2018 11:06

Make module.export = an alias like export= is

d2c86e4

This breaks a couple of tests for previous workarounds. Fix in upcoming commits.

Basically fixes all the breaks, but needs cleanup

5af0c77

More notes to myself

e27680c

Clean up TODOs

bfaa38d

Call mergeSymbolTable and delete export= afterward

9061184

instead of basically copying the code myself.

More cleanup

7dd3b6c

sandersn requested review from mhegazy and weswigham April 20, 2018 16:34

Merge branch 'master' into js/module-exports-is-alias

c2e2921

weswigham reviewed Apr 20, 2018

View reviewed changes

Remove unnecessary check in import type checking

465056a

sandersn mentioned this pull request Apr 20, 2018

Find All Reference from requiring file to module file works only if not default CommonJS exports #22222

Closed

weswigham mentioned this pull request Apr 23, 2018

feat(chunk): add chunk typing support webpack/webpack#7031

Merged

Revert to DIY code.

6ff110d

It is more correct and will go away in a few days.

Exported class expressions can be used as type

deb1e61

In both JS and TS

weswigham approved these changes Apr 23, 2018

View reviewed changes

Do not require named class expressions

7861de6

sandersn merged commit 905f9a0 into master Apr 23, 2018

ghost deleted the js/module-exports-is-alias branch May 1, 2018 19:38

ghost mentioned this pull request May 1, 2018

Different definition behavior for import vs require #23742

Closed

microsoft locked and limited conversation to collaborators Jul 31, 2018

Conversation

sandersn commented Apr 20, 2018

Uh oh!

weswigham Apr 20, 2018

Choose a reason for hiding this comment

Uh oh!

sandersn Apr 20, 2018

Choose a reason for hiding this comment

Uh oh!

weswigham Apr 20, 2018

Choose a reason for hiding this comment

Uh oh!

weswigham Apr 20, 2018

Choose a reason for hiding this comment

Uh oh!

sandersn Apr 20, 2018

Choose a reason for hiding this comment

Uh oh!

sandersn Apr 23, 2018

Choose a reason for hiding this comment

Uh oh!

sandersn Apr 23, 2018

Choose a reason for hiding this comment

Uh oh!

sandersn commented Apr 23, 2018

Uh oh!

sandersn commented Apr 23, 2018

Uh oh!

weswigham commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sandersn commented Apr 23, 2018

Uh oh!

weswigham commented Apr 23, 2018

Uh oh!

sandersn commented Apr 23, 2018

Uh oh!

weswigham left a comment

Choose a reason for hiding this comment

Uh oh!

sandersn commented Apr 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

weswigham commented Apr 23, 2018 •

edited

Loading