Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider the commonjs module indicator as a module indicator #18490

Merged
merged 2 commits into from
Nov 10, 2017
Merged

Consider the commonjs module indicator as a module indicator #18490

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df4f15d
Consider the commonjs module indicator as an indicator that something…
weswigham Sep 14, 2017
bc030f1
Only use commonjs module indicator when targeting commonjs
weswigham Nov 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/compiler/transformers/module/module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ namespace ts {
* @param node The SourceFile node.
*/
function transformSourceFile(node: SourceFile) {
if (node.isDeclarationFile || !(isExternalModule(node) || compilerOptions.isolatedModules || node.transformFlags & TransformFlags.ContainsDynamicImport)) {
if (node.isDeclarationFile || !(isEffectiveExternalModule(node, compilerOptions) || node.transformFlags & TransformFlags.ContainsDynamicImport)) {
return node;
}

Expand Down
2 changes: 1 addition & 1 deletion src/compiler/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,7 +444,7 @@ namespace ts {
}

export function isEffectiveExternalModule(node: SourceFile, compilerOptions: CompilerOptions) {
return isExternalModule(node) || compilerOptions.isolatedModules;
return isExternalModule(node) || compilerOptions.isolatedModules || !!node.commonJsModuleIndicator;
Copy link
Contributor

@mhegazy mhegazy Oct 12, 2017 •
edited by weswigham
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems like the best compromise here:
(!!node.commonJsModuleIndicator && getEmitModuleKind(compilerOptions) === ModuleKind.CommonJS)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so what u think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems reasonable. Though anyone with mixed-mode compilations targeting commonjs where they use require in some global files is going to get sad when their global files start getting transpiled as modules. Though why we even support mixed mode compilations in the first place...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the other option is to do nothing and leave the current behavior.. which is broken on import helpers + js files (which could be a smaller group affected than by the proposed change)...

Copy link
Member Author

@weswigham weswigham Nov 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmmmmmm I think once limited to commonjs it's likely fine. The only difference other than helpers, which is what this aims to change, for module vs non-module would be that top level declarations are local to the file instead of global. And given that the commonjs module flag is only set in JS files, it's not like there'll be any potential interface or namespace merging going on; so it'd just be weather people expected the var x they declared in one file with a require to be visible in another file with a require..... except that's governed by isExternalModule and not isEffectiveExternalModule (since the latter only affects emit), so I'm pretty sure once limited to commonjs there's no obserable changes?

}

/* @internal */
Expand Down
9 changes: 6 additions & 3 deletions tests/baselines/reference/dynamicRequire.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ function foo(name) {
}

//// [a_out.js]
function foo(name) {
var s = require("t/" + name);
}
define("a", ["require", "exports"], function (require, exports) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm.. this does not look good. we should not mess up the module output if we can.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure what to do though.. @rbuckton thoughts?

Copy link
Member Author

@weswigham weswigham Oct 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file has an explicit @module: amd BTW (this could be seen as an improvement). Our module emit and our global emit for commonjs are pretty much identical - the only difference is in how the typechecker sees the top-level scope.

"use strict";
function foo(name) {
var s = require("t/" + name);
}
});
23 changes: 23 additions & 0 deletions tests/baselines/reference/javascriptCommonjsModule.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
//// [index.js]
class Foo {}

class Bar extends Foo {}

module.exports = Bar;


//// [index.js]
var tslib_1 = require("tslib");
var Foo = /** @class */ (function () {
function Foo() {
}
return Foo;
}());
var Bar = /** @class */ (function (_super) {
tslib_1.__extends(Bar, _super);
function Bar() {
return _super !== null && _super.apply(this, arguments) || this;
}
return Bar;
}(Foo));
module.exports = Bar;
13 changes: 13 additions & 0 deletions tests/baselines/reference/javascriptCommonjsModule.symbols
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
=== tests/cases/compiler/index.js ===
class Foo {}
>Foo : Symbol(Foo, Decl(index.js, 0, 0))

class Bar extends Foo {}
>Bar : Symbol(Bar, Decl(index.js, 0, 12))
>Foo : Symbol(Foo, Decl(index.js, 0, 0))

module.exports = Bar;
>module : Symbol(export=, Decl(index.js, 2, 24))
>exports : Symbol(export=, Decl(index.js, 2, 24))
>Bar : Symbol(Bar, Decl(index.js, 0, 12))

15 changes: 15 additions & 0 deletions tests/baselines/reference/javascriptCommonjsModule.types
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
=== tests/cases/compiler/index.js ===
class Foo {}
>Foo : Foo

class Bar extends Foo {}
>Bar : Bar
>Foo : Foo

module.exports = Bar;
>module.exports = Bar : typeof Bar
>module.exports : any
>module : any
>exports : any
>Bar : typeof Bar

23 changes: 13 additions & 10 deletions ...e/project/nodeModulesMaxDepthExceeded/amd/maxDepthExceeded/built/node_modules/m1/index.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion ...roject/nodeModulesMaxDepthExceeded/amd/maxDepthExceeded/built/node_modules/m1/relative.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions tests/cases/compiler/javascriptCommonjsModule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// @allowJS: true
// @outDir: ./out
// @module: commonjs
// @noEmitHelpers: true
// @importHelpers: true
// @filename: index.js
class Foo {}

class Bar extends Foo {}

module.exports = Bar;