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.

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

tsc has "module" default to "es2015", but ts-loader does not #570

Closed
Venryx opened this issue Jun 30, 2017 · 7 comments
Closed

tsc has "module" default to "es2015", but ts-loader does not #570

Venryx opened this issue Jun 30, 2017 · 7 comments

Comments

@Venryx
Copy link

Venryx commented Jun 30, 2017

Title basically says it all.

Here is my config:

{
	"compilerOptions": {
		"sourceMap": true,
		"watch": true,
		"outDir": "Source_JS",
		"forceConsistentCasingInFileNames": true,



		// this is the important line
		"module": "es2015",



		"moduleResolution": "node",
		"rootDir": "Source",
		"baseUrl": "Source",
		"paths": {
			"*": [
				"../node_modules/@types/*",
				"*"
			]
		},
		"target": "esnext",
		"lib": [
			"es6",
			"es5",
			"dom"
		],
		"allowJs": true,
		"jsx": "react",
		"noImplicitAny": false,
		"experimentalDecorators": true,
		"allowSyntheticDefaultImports": true
	},
	"files": ["Source/Main.ts"],
	"include": [
		"Typings/**/*.d.ts",
		"Source/**/*.ts",
		"Source/**/*.tsx"
	],
	"exclude": [
		"Build",
		"Tests",
		"node_modules"
	],
	"compileOnSave": true
}

I had to add the module: "es2015" line, because the handling was different between tsc and ts-loader, and it was causing problems in my project.

It was confusing because I expected an identical config file to give the same results in both tsc and ts-loader.

For reference, the difference in output between ts-loader and tsc when the "module" config was not specified, is that tsc would (correctly) output:

var _ajv = __webpack_require__(614);
var _ajv2 = _interopRequireDefault(_ajv);
var instance = new _ajv2.default();

Whereas ts-loader would output:

var _ajv = __webpack_require__(651);
var instance = new _ajv.default();

The first output is correct since "babel-plugin-transform-es2015-modules-commonjs" with default settings is enabled in the project.

I believe this can be fixed by just having the default value for "module" match that of tsc (by being "es2015"), for when it's not specified in tsconfig.json (as was the case in my project).

@johnnyreilly
Copy link
Member

Thanks for raising this! I appreciate the detail. Could you confirm when this became the case please? I ask as ts-loader predates module: "es2015" being supported by tsc. Since ts-loader supports TypeScript all the way back to 1.6 we don't want to break usage for those users.

Also, is it always the default? My guess is not and that it becomes the default if certain other compiler options are set. We'd like to reflect tsc's behaviour as much as possible. Could you clarify in what circumstances we should make this the default please?

To summarize: I think this is a change worth making but we want to make it correctly 😄

@Venryx
Copy link
Author

Venryx commented Jun 30, 2017

Well, Typescript's compiler-options page shows this: (the third column is for the "default")

So it appears that tsc is using "es2015" as the default, but only because the target is "ES6".

In my project, "target" is actually set to "esnext". But I assume this also satisfies tsc's requirement (despite the page saying "=== es6"), since that seems to be the resulting behavior.

In other words, it seems that to match tsc's behavior, "module" should default to "es2015" when "target" is set to "es6" -- OR when it's set to "es2015", "esnext", etc.

However, if that's true, it goes against that reference page, which only lists "es6" as matching. So I guess it would need to be confirmed that tsc's current behavior is what is actually intended.

@Venryx
Copy link
Author

Venryx commented Jun 30, 2017

I dug into Typescript's source-code, and indeed, if "target" is set to any of ["es6", "es2015", "es2016", "es2017", "esNext"], then "module" defaults to "es6".

Here are the files where this can be seen:

Enum declaration: (from: https://github.com/Microsoft/TypeScript/blob/ac72803bb2b299e6b331329a9781cb73cc55c156/src/compiler/core.ts#L1595)

    const enum ScriptTarget {
        ES3 = 0,
        ES5 = 1,
        ES2015 = 2,
        ES2016 = 3,
        ES2017 = 4,
        ESNext = 5,
        Latest = 5,
    }

Module default-value logic: (from: https://github.com/Microsoft/TypeScript/blob/73ee2feb51c9b7e24a29eb4cee19d7c14b933065/lib/tsserverlibrary.d.ts#L2117)

    export function getEmitModuleKind(compilerOptions: CompilerOptions) {
        return typeof compilerOptions.module === "number" ?
            compilerOptions.module :
            getEmitScriptTarget(compilerOptions) >= ScriptTarget.ES2015 ? ModuleKind.ES2015 : ModuleKind.CommonJS;
    }

So the question is, should ts-loader match tsc's actual behavior (checking for es6 or higher), as shown above?

Or should it stick with what is shown on Typescript's reference page (checking for es6 only), contrary to tsc?

It seems to me that it should do the former, and someone should just inform the reference-page authors to update their page. (EDIT: done)

For reference, here is ts-loader's version of the module default-value logic: (from: https://github.com/TypeStrong/ts-loader/blob/master/src/compilerSetup.ts#L53)

    // if `module` is not specified and not using ES6 target, default to CJS module output
    if ((!compilerOptions.module) && compilerOptions.target !== constants.ScriptTargetES2015) {
        compilerOptions.module = constants.ModuleKindCommonJs;
    } else if (compilerCompatible && semver.lt(compiler.version, '1.7.3-0') && compilerOptions.target === constants.ScriptTargetES2015) {
        // special handling for TS 1.6 and target: es6
        compilerOptions.module = constants.ModuleKindNone;
    }

@johnnyreilly
Copy link
Member

So the question is, should ts-loader match tsc's actual behavior (checking for es6 or higher), as shown above?

Yes I think it should do this - their docs are just out of date I guess. Thanks for notifying them about the docs too.

If you'd like to submit a PR which made this change it would be greatly appreciated!

@Venryx
Copy link
Author

Venryx commented Aug 8, 2017

It seems the solution would be to just make these two changes:

    // if `module` is not specified and not using ES6 target, default to CJS module output
-    if ((!compilerOptions.module) && compilerOptions.target !== constants.ScriptTargetES2015) {
+    if ((!compilerOptions.module) && compilerOptions.target < constants.ScriptTargetES2015) {
        compilerOptions.module = constants.ModuleKindCommonJs;
-    } else if (compilerCompatible && semver.lt(compiler.version, '1.7.3-0') && compilerOptions.target === constants.ScriptTargetES2015) {
+    } else if (compilerCompatible && semver.lt(compiler.version, '1.7.3-0') && compilerOptions.target >= constants.ScriptTargetES2015) {
        // special handling for TS 1.6 and target: es6
        compilerOptions.module = constants.ModuleKindNone;
    }

I have not tested it, but it should be correct, as that's the logic used by tsc. (as seen in my previous post)

@johnnyreilly
Copy link
Member

Thanks - might give it a try!

@johnnyreilly
Copy link
Member

Include this with v3 changes #631

@johnnyreilly johnnyreilly mentioned this issue Sep 24, 2017
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants