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

Synthesize namespace records for proper esm interop #19675

Merged
merged 32 commits into from
Jan 9, 2018
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
213146d
Integrate importStar and importDefault helpers
weswigham Nov 1, 2017
b5a7ef9
Accept baselines
weswigham Nov 1, 2017
8abc907
Support dynamic imports, write helpers for umd module (and amd is pos…
weswigham Nov 1, 2017
2d0c8d3
Accept baselines
weswigham Nov 1, 2017
a725916
Support AMD, use same helper initialization as is normal
weswigham Nov 2, 2017
e910603
update typechecker to have errors on called imported namespaces and g…
weswigham Nov 2, 2017
e84b051
Overhaul allowSyntheticDefaultExports to be safer
weswigham Nov 3, 2017
f23d41c
Put the new behavior behind a flag
weswigham Nov 3, 2017
ebd4c03
Merge branch 'master' into module-nodejs
weswigham Nov 7, 2017
3b33df0
Rename strictESM to ESMInterop
weswigham Nov 7, 2017
7ff11bb
Merge branch 'master' into module-nodejs
weswigham Nov 10, 2017
bcafdba
ESMInterop -> ESModuleInterop, make default for tsc --init
weswigham Nov 10, 2017
eaf2fc5
Merge branch 'master' into module-nodejs
weswigham Nov 17, 2017
29c731c
Rename ESMInterop -> ESModuleInterop in module.ts, add emit test (sin…
weswigham Nov 17, 2017
4350caf
Merge branch 'master' into module-nodejs
weswigham Nov 17, 2017
e20a548
Remove erroneous semicolons from helper
weswigham Nov 18, 2017
578141d
Merge branch 'master' into module-nodejs
weswigham Nov 28, 2017
dfe5675
Reword diagnostic
weswigham Nov 29, 2017
8c6c313
Change style
weswigham Nov 29, 2017
3d996d7
Edit followup diagnostic
weswigham Nov 29, 2017
2f9c240
Add secondary quickfix for call sites, tests forthcoming
weswigham Nov 29, 2017
2361f37
Add synth default to namespace import type, enhance quickfix
weswigham Nov 29, 2017
357996b
Pair of spare tests for good measure
weswigham Nov 29, 2017
a682476
Merge branch 'master' into module-nodejs
weswigham Dec 18, 2017
6e9e874
Fix typos in diagnostic message
weswigham Jan 5, 2018
a654b82
Improve comment clarity
weswigham Jan 5, 2018
ef6faf1
Actually accept the updated changes to the esmodule interop description
weswigham Jan 8, 2018
a8233b3
ESModule -> esModule
weswigham Jan 8, 2018
f4f4e84
Use find and not forEach
weswigham Jan 8, 2018
386c54d
Use guard
weswigham Jan 8, 2018
2663db7
Rely on implicit falsiness of Result.False
weswigham Jan 8, 2018
8068e2e
These should have been emit flags
weswigham Jan 8, 2018
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
140 changes: 124 additions & 16 deletions src/compiler/checker.ts

Large diffs are not rendered by default.

10 changes: 9 additions & 1 deletion src/compiler/commandLineParser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,13 @@ namespace ts {
category: Diagnostics.Module_Resolution_Options,
description: Diagnostics.Allow_default_imports_from_modules_with_no_default_export_This_does_not_affect_code_emit_just_typechecking
},
{
name: "ESModuleInterop",
type: "boolean",
showInSimplifiedHelpView: true,
category: Diagnostics.Module_Resolution_Options,
description: Diagnostics.Enables_emit_interoperability_bewteen_commonjs_and_ES_Modules_via_creation_of_namespace_objects_for_all_imports_Implies_allowSyntheticDefaultImports
},
{
name: "preserveSymlinks",
type: "boolean",
Expand Down Expand Up @@ -700,7 +707,8 @@ namespace ts {
export const defaultInitCompilerOptions: CompilerOptions = {
module: ModuleKind.CommonJS,
target: ScriptTarget.ES5,
strict: true
strict: true,
ESModuleInterop: true
};

let optionNameMapCache: OptionNameMap;
Expand Down
4 changes: 3 additions & 1 deletion src/compiler/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1966,7 +1966,9 @@ namespace ts {
const moduleKind = getEmitModuleKind(compilerOptions);
return compilerOptions.allowSyntheticDefaultImports !== undefined
? compilerOptions.allowSyntheticDefaultImports
: moduleKind === ModuleKind.System;
: compilerOptions.ESModuleInterop
Copy link
Contributor

Choose a reason for hiding this comment

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

should we make it an error to specify --ESModuleInterop with --module >= ES2015?

Copy link
Member Author

Choose a reason for hiding this comment

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

When module >= ES2015, it's just a synonym for allowSyntheticDefaultImports. IMO it can be left as not-an-error, since you may still want it for interop if, eg, you're using babel for emit.

Copy link
Contributor

Choose a reason for hiding this comment

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

but our output does not handle that case, and --allowSyntheticDefaultImports already does that..

? moduleKind !== ModuleKind.None && moduleKind < ModuleKind.ES2015
: moduleKind === ModuleKind.System;
}

export type StrictOptionName = "noImplicitAny" | "noImplicitThis" | "strictNullChecks" | "strictFunctionTypes" | "strictPropertyInitialization" | "alwaysStrict";
Expand Down
12 changes: 12 additions & 0 deletions src/compiler/diagnosticMessages.json
Original file line number Diff line number Diff line change
Expand Up @@ -3519,6 +3519,14 @@
"category": "Error",
"code": 7036
},
"Enables emit interoperability bewteen commonjs and ES Modules via creation of namespace objects for all imports. Implies `allowSyntheticDefaultImports`.": {
Copy link
Member

Choose a reason for hiding this comment

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

between

CommonJS

Stick with consistent inner quote marks - we usually use apostrophes/single-quotes.

"category": "Message",
"code": 7037
},
"A namespace-style import cannot be called or constructed, and will cause a failure at runtime.": {
"category": "Error",
"code": 7038
},

"You cannot rename this element.": {
"category": "Error",
Expand Down Expand Up @@ -3886,5 +3894,9 @@
"Install '{0}'": {
"category": "Message",
"code": 95014
},
"Replace import with '{0}'.": {
"category": "Message",
"code": 95015
}
}
1 change: 1 addition & 0 deletions src/compiler/program.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1586,6 +1586,7 @@ namespace ts {
// synthesize 'import "tslib"' declaration
const externalHelpersModuleReference = createLiteral(externalHelpersModuleNameText);
const importDecl = createImportDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined, /*importClause*/ undefined);
importDecl.transformFlags |= TransformFlags.NeverApplyImportHelper;
Copy link
Member

Choose a reason for hiding this comment

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

The only place that should be setting transformFlags is computeTransformFlagsForNode in binder.ts (the only notable exception is convertIterationStatementBodyIfNecessary in es2015.ts, which I'd eventually like to fix as well). This seems like it should be on EmitFlags instead.

externalHelpersModuleReference.parent = importDecl;
importDecl.parent = file;
imports = [externalHelpersModuleReference];
Expand Down
14 changes: 7 additions & 7 deletions src/compiler/transformers/module/es2015.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,14 @@ namespace ts {
if (externalHelpersModuleName) {
const statements: Statement[] = [];
const statementOffset = addPrologue(statements, node.statements);
append(statements,
createImportDeclaration(
/*decorators*/ undefined,
/*modifiers*/ undefined,
createImportClause(/*name*/ undefined, createNamespaceImport(externalHelpersModuleName)),
createLiteral(externalHelpersModuleNameText)
)
const tslibImport = createImportDeclaration(
/*decorators*/ undefined,
/*modifiers*/ undefined,
createImportClause(/*name*/ undefined, createNamespaceImport(externalHelpersModuleName)),
createLiteral(externalHelpersModuleNameText)
);
tslibImport.transformFlags |= TransformFlags.NeverApplyImportHelper;
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't set transform flags explicitly. See my related comment in program.ts.

append(statements, tslibImport);

addRange(statements, visitNodes(node.statements, visitor, isStatement, statementOffset));
return updateSourceFileNode(
Expand Down
82 changes: 76 additions & 6 deletions src/compiler/transformers/module/module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ namespace ts {
// Create an updated SourceFile:
//
// define(moduleName?, ["module1", "module2"], function ...
return updateSourceFileNode(node,
const updated = updateSourceFileNode(node,
setTextRange(
createNodeArray([
createStatement(
Expand Down Expand Up @@ -192,6 +192,9 @@ namespace ts {
/*location*/ node.statements
)
);

addEmitHelpers(updated, context.readEmitHelpers());
return updated;
}

/**
Expand Down Expand Up @@ -296,7 +299,7 @@ namespace ts {
// }
// })(function ...)

return updateSourceFileNode(
const updated = updateSourceFileNode(
node,
setTextRange(
createNodeArray([
Expand Down Expand Up @@ -328,6 +331,9 @@ namespace ts {
/*location*/ node.statements
)
);

addEmitHelpers(updated, context.readEmitHelpers());
return updated;
}

/**
Expand Down Expand Up @@ -385,6 +391,18 @@ namespace ts {
return { aliasedModuleNames, unaliasedModuleNames, importAliasNames };
}

function getAMDImportExpressionForImport(node: ImportDeclaration | ExportDeclaration | ImportEqualsDeclaration) {
if (isImportEqualsDeclaration(node) || isExportDeclaration(node) || !getExternalModuleNameLiteral(node, currentSourceFile, host, resolver, compilerOptions)) {
return undefined;
}
const name = getLocalNameForExternalImport(node, currentSourceFile);
const expr = getHelperExpressionForImport(node, name);
if (expr === name) {
return undefined;
}
return createStatement(createAssignment(name, expr));
}

/**
* Transforms a SourceFile into an AMD or UMD module body.
*
Expand All @@ -402,6 +420,9 @@ namespace ts {

// Visit each statement of the module body.
append(statements, visitNode(currentModuleInfo.externalHelpersImportDeclaration, sourceElementVisitor, isStatement));
if (moduleKind === ModuleKind.AMD) {
addRange(statements, mapDefined(currentModuleInfo.externalImports, getAMDImportExpressionForImport));
}
addRange(statements, visitNodes(node.statements, sourceElementVisitor, isStatement, statementOffset));

// Append the 'export =' statement if provided.
Expand Down Expand Up @@ -617,7 +638,12 @@ namespace ts {
}
}

return createNew(createIdentifier("Promise"), /*typeArguments*/ undefined, [func]);
const promise = createNew(createIdentifier("Promise"), /*typeArguments*/ undefined, [func]);
if (compilerOptions.ESModuleInterop) {
context.requestEmitHelper(importStarHelper);
return createCall(createPropertyAccess(promise, createIdentifier("then")), /*typeArguments*/ undefined, [getHelperName("__importStar")]);
}
return promise;
}

function createImportCallExpressionCommonJS(arg: Expression | undefined, containsLexicalThis: boolean): Expression {
Expand All @@ -627,7 +653,11 @@ namespace ts {
// We have to wrap require in then callback so that require is done in asynchronously
// if we simply do require in resolve callback in Promise constructor. We will execute the loading immediately
const promiseResolveCall = createCall(createPropertyAccess(createIdentifier("Promise"), "resolve"), /*typeArguments*/ undefined, /*argumentsArray*/ []);
const requireCall = createCall(createIdentifier("require"), /*typeArguments*/ undefined, arg ? [arg] : []);
let requireCall = createCall(createIdentifier("require"), /*typeArguments*/ undefined, arg ? [arg] : []);
if (compilerOptions.ESModuleInterop) {
context.requestEmitHelper(importStarHelper);
requireCall = createCall(getHelperName("__importStar"), /*typeArguments*/ undefined, [requireCall]);
}

let func: FunctionExpression | ArrowFunction;
if (languageVersion >= ScriptTarget.ES2015) {
Expand Down Expand Up @@ -660,6 +690,22 @@ namespace ts {
return createCall(createPropertyAccess(promiseResolveCall, "then"), /*typeArguments*/ undefined, [func]);
}


function getHelperExpressionForImport(node: ImportDeclaration, innerExpr: Expression) {
if (!compilerOptions.ESModuleInterop || node.transformFlags & TransformFlags.NeverApplyImportHelper) {
return innerExpr;
}
if (getNamespaceDeclarationNode(node)) {
context.requestEmitHelper(importStarHelper);
return createCall(getHelperName("__importStar"), /*typeArguments*/ undefined, [innerExpr]);
}
if (isDefaultImport(node)) {
context.requestEmitHelper(importDefaultHelper);
return createCall(getHelperName("__importDefault"), /*typeArguments*/ undefined, [innerExpr]);
}
return innerExpr;
}

/**
* Visits an ImportDeclaration node.
*
Expand All @@ -681,7 +727,7 @@ namespace ts {
createVariableDeclaration(
getSynthesizedClone(namespaceDeclaration.name),
/*type*/ undefined,
createRequireCall(node)
getHelperExpressionForImport(node, createRequireCall(node))
)
);
}
Expand All @@ -694,7 +740,7 @@ namespace ts {
createVariableDeclaration(
getGeneratedNameForNode(node),
/*type*/ undefined,
createRequireCall(node)
getHelperExpressionForImport(node, createRequireCall(node))
)
);

Expand Down Expand Up @@ -1671,4 +1717,28 @@ namespace ts {
text: `
var __syncRequire = typeof module === "object" && typeof module.exports === "object";`
};

// emit helper for `import * as Name from "foo"`
const importStarHelper: EmitHelper = {
name: "typescript:commonjsimportstar",
scoped: false,
text: `
var __importStar = (this && this.__importStar) || function (mod) {
if (mod && mod.__esModule) return mod;
var result = {};
if (mod != null) for (var k in mod) if (Object.hasOwnProperty.call(mod, k)) result[k] = mod[k];
result["default"] = mod;
return result;
}`
};

// emit helper for `import Name from "foo"`
const importDefaultHelper: EmitHelper = {
name: "typescript:commonjsimportdefault",
scoped: false,
text: `
var __importDefault = (this && this.__importDefault) || function (mod) {
return (mod && mod.__esModule) ? mod : { "default": mod };
}`
};
}
1 change: 1 addition & 0 deletions src/compiler/transformers/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ namespace ts {
createLiteral(externalHelpersModuleNameText));

if (externalHelpersImportDeclaration) {
externalHelpersImportDeclaration.transformFlags |= TransformFlags.NeverApplyImportHelper;
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't set transform flags explicitly. See my related comment in program.ts.

externalImports.unshift(externalHelpersImportDeclaration);
}

Expand Down
4 changes: 4 additions & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3165,6 +3165,7 @@ namespace ts {
bindingElement?: BindingElement; // Binding element associated with property symbol
exportsSomeValue?: boolean; // True if module exports some value (not just types)
enumKind?: EnumKind; // Enum declaration classification
originatingImport?: ImportDeclaration | ImportCall; // Import declaration which produced the symbol, present if the symbol is poisoned
Copy link
Member

Choose a reason for hiding this comment

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

I know what do you mean by poisoned, but will a future reader?

lateSymbol?: Symbol; // Late-bound symbol for a computed property
}

Expand Down Expand Up @@ -3877,6 +3878,7 @@ namespace ts {
typeRoots?: string[];
/*@internal*/ version?: boolean;
/*@internal*/ watch?: boolean;
ESModuleInterop?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

The capitalization of this option doesn't align with other options. Should this instead be esModuleInterop?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough, there's prior work in the form of jsxFactory for downcasing abbreviations that lead the name. I'll change it.


[option: string]: CompilerOptionsValue | JsonSourceFile | undefined;
}
Expand Down Expand Up @@ -4344,6 +4346,7 @@ namespace ts {
ContainsYield = 1 << 24,
ContainsHoistedDeclarationOrCompletion = 1 << 25,
ContainsDynamicImport = 1 << 26,
NeverApplyImportHelper = 1 << 27, // Indicates the node should never be wrapped with an import star helper (because, for example, it import tslib itself)
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be on EmitFlags instead?


// Please leave this as 1 << 29.
// It is the maximum bit we can set before we outgrow the size of a v8 small integer (SMI) on an x86 system.
Expand Down Expand Up @@ -4383,6 +4386,7 @@ namespace ts {
// - Additional bitmasks
TypeScriptClassSyntaxMask = ContainsParameterPropertyAssignments | ContainsPropertyInitializer | ContainsDecorators,
ES2015FunctionSyntaxMask = ContainsCapturedLexicalThis | ContainsDefaultValueAssignments,
StickyFlags = NeverApplyImportHelper, // Flags which once set are retained by `aggregateTransformFlags` instead of overwritten
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't need this if NeverApplyImportHelper is moved to EmitFlags.

}

export interface SourceMapRange extends TextRange {
Expand Down
2 changes: 2 additions & 0 deletions src/compiler/visitor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1466,7 +1466,9 @@ namespace ts {
* Aggregates the TransformFlags for a Node and its subtree.
*/
export function aggregateTransformFlags<T extends Node>(node: T): T {
const stickyFlags = node.transformFlags & TransformFlags.StickyFlags;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is a good approach. NeverApplyImportHelper should be on EmitFlags.

aggregateTransformFlagsForNode(node);
node.transformFlags |= stickyFlags;
return node;
}

Expand Down
52 changes: 52 additions & 0 deletions src/services/codefixes/fixInvalidImportSyntax.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/* @internal */
namespace ts.codefix {
registerCodeFix({
errorCodes: [Diagnostics.A_namespace_style_import_cannot_be_called_or_constructed_and_will_cause_a_failure_at_runtime.code],
getCodeActions: getActionsForInvalidImport
});

function getActionsForInvalidImport(context: CodeFixContext): CodeAction[] | undefined {
const sourceFile = context.sourceFile;

// This is the whole import statement, eg:
// import * as Bluebird from 'bluebird';
// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
const node = getTokenAtPosition(sourceFile, context.span.start, /*includeJsDocComment*/ false).parent as ImportDeclaration;
const namespace = getNamespaceDeclarationNode(node) as NamespaceImport;
const opts = context.program.getCompilerOptions();
const variations: CodeAction[] = [];
if (opts.module === ModuleKind.CommonJS || (!opts.module && opts.target < ScriptTarget.ES2015)) {
// import Bluebird = require("bluebird");
const replacement = createImportEqualsDeclaration(
/*decorators*/ undefined,
/*modifiers*/ undefined,
namespace.name,
createExternalModuleReference(node.moduleSpecifier)
);
const changeTracker = textChanges.ChangeTracker.fromContext(context);
changeTracker.replaceNode(sourceFile, node, replacement, { useNonAdjustedEndPosition: true });
const changes = changeTracker.getChanges();
variations.push({
description: formatStringFromArgs(getLocaleSpecificMessage(Diagnostics.Replace_import_with_0), [changes[0].textChanges[0].newText]),
changes
});
}

// import Bluebird from "bluebird";
const replacement = createImportDeclaration(
/*decorators*/ undefined,
/*modifiers*/ undefined,
createImportClause(namespace.name, /*namedBindings*/ undefined),
node.moduleSpecifier
);
const changeTracker = textChanges.ChangeTracker.fromContext(context);
changeTracker.replaceNode(sourceFile, node, replacement, { useNonAdjustedEndPosition: true });
const changes = changeTracker.getChanges();
variations.push({
description: formatStringFromArgs(getLocaleSpecificMessage(Diagnostics.Replace_import_with_0), [changes[0].textChanges[0].newText]),
changes
});

return variations;
}
}
2 changes: 2 additions & 0 deletions src/services/codefixes/fixes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,5 @@
/// <reference path='disableJsDiagnostics.ts' />
/// <reference path='helpers.ts' />
/// <reference path='inferFromUsage.ts' />
/// <reference path="fixInvalidImportSyntax.ts" />

11 changes: 1 addition & 10 deletions tests/baselines/reference/allowSyntheticDefaultImports1.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,12 @@
import Namespace from "./b";
export var x = new Namespace.Foo();

//// [b.ts]
//// [b.d.ts]
export class Foo {
member: string;
}


//// [b.js]
"use strict";
exports.__esModule = true;
var Foo = /** @class */ (function () {
function Foo() {
}
return Foo;
}());
exports.Foo = Foo;
//// [a.js]
"use strict";
exports.__esModule = true;
Expand Down
Loading