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

Jump to bottom

Add refactoring to convert CommonJS module to ES6 module #19916

Merged

7 commits merged into master from convertToEsModule

Jan 9, 2018

ghost commented Nov 10, 2017

Fixes #19719

ghost requested a review from amcasey

November 10, 2017 17:44

ghost force-pushed the convertToEsModule branch 2 times, most recently from 4118c0c to 517715a Compare

November 10, 2017 18:58


          Add refactoring to convert CommonJS module to ES6 module

a4b588d

ghost force-pushed the convertToEsModule branch from 517715a to a4b588d Compare

November 10, 2017 19:00

Member

amcasey commented Nov 13, 2017

Someone who understands modules should probably take a look too.

Author

ghost commented Nov 13, 2017

@uniqueiniquity @minestarks @armanio123 @aozgaa Someone want to take this on?

amcasey reviewed

View reviewed changes

Member

amcasey left a comment

Notes on all files but convertToEs6Module.ts.

tests/cases/fourslash/refactorConvertToEs6Module_export_alias.ts Outdated

		@@ -0,0 +1,20 @@
		/// <reference path='fourslash.ts' />

		// Test that we leave it alone if the name is a keyword.

Member

amcasey Nov 13, 2017

Copy-paste error?

tests/cases/fourslash/refactorConvertToEs6Module_export_referenced.ts

+              const y = 1;
+              const _y = y;
+              export { _y as y };

Member

amcasey Nov 13, 2017

Presumably, there's no conflict between this y and the constant above?

Author

ghost Nov 14, 2017

Right, the y in as y is the exported name but isn't a local variable.

tests/cases/fourslash/refactorConvertToEs6Module_import_multipleUniqueIdentifiers.ts

+                  newContent: `import x from "x";
+              import _x from "x";
+              const [a, b] = _x;
+              import __x from "x";

Member

amcasey Nov 13, 2017

Are imports supposed to go at the top?

Author

ghost Nov 14, 2017

Some people lint for that style but it's not a language rule. In this case I think it makes sense to put the only use of _x next to it. It's going to be ugly anyway so hopefully people aren't doing unusual destructuring of a require().

tests/cases/fourslash/refactorConvertToEs6Module_import_propertyAccess.ts Outdated

+              import { d } from "c";
+              const [a, b] = d;
+              import { a as _a } from "c";
+              const [a, b] = _a; // Test that we avoid shadowing 'a'`,

Member

amcasey Nov 13, 2017

This comment could be clearer about which a is (not) being shadowed.

src/compiler/core.ts Outdated

@@ @@ -394,6 +394,16 @@ namespace ts { @@
                       return result;
                   }
+                  export function mapIter<T, U>(iter: Iterator<T>, mapFn: (x: T) => U): U[] {

Member

amcasey Nov 13, 2017

Naively, I would have expected this to return an Iterator.

src/compiler/core.ts

@@ @@ -515,12 +525,23 @@ namespace ts { @@
                       return result || array;
                   }
+                  export function mapAllOrFail<T, U>(array: ReadonlyArray<T>, mapFn: (x: T, i: number) => U | undefined): U[] | undefined {

Member

amcasey Nov 13, 2017

You can pass a function that only takes x, right?

Author

ghost Nov 14, 2017

👍

src/compiler/core.ts Outdated

+                      const result: U[] = [];
+                      for (let i = 0; i < array.length; i++) {
+                          const mapped = mapFn(array[i], i);
+                          if (!mapped) {

Member

amcasey Nov 13, 2017

Should this be an explicit check for undefined? Or is a falsy value considered to indicate failure?

amcasey approved these changes

View reviewed changes

Member

amcasey left a comment

Seems reasonable, but I don't understand it deeply.

src/services/refactors/convertToEs6Module.ts Outdated

+                   * Contains an entry for each renamed export.
+                   * This is necessary because `exports.x = 0;` does not declare a local variable.
+                   * Converting this to `export const x = 0;` would declare a local, so we must be careful to avoid shadowing.
+                   * If there would be shadowing at either the declaration or at any reference to `exports.x` (now just `x`), we must conver to:

Member

amcasey Nov 13, 2017

Typo "conver to"

src/services/refactors/convertToEs6Module.ts Outdated

+                      });
+                  }
+                  function forEachExportReference(sourceFile: SourceFile, cb: (node: PropertyAccessExpression, isAssignment: boolean) => void): void {

Member

amcasey Nov 13, 2017

isAssignment means that node is the LHS of an assignment?

src/services/refactors/convertToEs6Module.ts

+                  }
+                  /** Whether `module.exports =` was changed to `export default` */
+                  type ModuleExportsChanged = boolean;

Member

amcasey Nov 13, 2017

Out of curiosity, why alias boolean like this?

Author

ghost Nov 14, 2017

So I don't have to keep documenting what the return value means -- convertAssignment(...): boolean doesn't really indicate that well.

src/services/refactors/convertToEs6Module.ts Outdated

+                      });
+                      if (foundImport) {
+                          // useNonAdjustedEndPosition to ensure we don't eat the newline after the statement.
+                          changes.replaceNodeWithNodes(sourceFile, statement, newNodes, { nodeSeparator: "\n", useNonAdjustedEndPosition: true });

Member

amcasey Nov 13, 2017

I think the refactoring context might contain a newline character.

src/services/refactors/convertToEs6Module.ts

+                          switch (prop.kind) {
+                              case SyntaxKind.GetAccessor:
+                              case SyntaxKind.SetAccessor:
+                              // TODO: Maybe we should handle this? See fourslash test `refactorConvertToEs6Module_export_object_shorthand.ts`.

Member

amcasey Nov 13, 2017

I'm not sure I understand what work remains to be done here.

Author

ghost Nov 14, 2017 •

edited by ghost

Loading

We could transpile that test to just export function f() {} instead of function f() {} export default { f };. But it would be work, since we would have to go backwards from the module.exports = statement to get a set of nodes that need to be changed, and I don't know how common it is to write a module like that.

src/services/refactors/convertToEs6Module.ts

+                              case SyntaxKind.CallExpression: {
+                                  const call = parent as CallExpression;
+                                  if (isRequireCall(call, /*checkArgumentIsStringLiteral*/ false)) {
+                                      changes.replaceNode(importingFile, parent, createPropertyAccess(getSynthesizedDeepClone(call), "default"));

Member

amcasey Nov 13, 2017

I would have guessed there was a constant for "default" somewhere.

Author

ghost Nov 14, 2017

Doesn't look like it, "default" occurs 67 times in our code. You could make a separate PR for that.

src/services/refactors/convertToEs6Module.ts

+                          case SyntaxKind.ClassExpression:
+                              // `exports.C = class {}` --> `export class C {}`
+                              return classExpressionToDeclaration(name, modifiers, exported as ClassExpression);
+                          default:

Member

amcasey Nov 13, 2017

Does this cover a reasonably short list of kinds? It would be nice if we could have the compiler notify us when a new case requires coverage.

Author

ghost Nov 14, 2017

Unfortunately Expression is a huge list of kinds. But only function/class expressions can be converted to declarations, all others should be left as expressions.

src/services/refactors/convertToEs6Module.ts Outdated

+                          const { text, originalKeywordKind } = node.name;
+                          if (!res.has(text) && (originalKeywordKind !== undefined && isNonContextualKeyword(originalKeywordKind)
+                              || checker.resolveName(node.name.text, node, SymbolFlags.Value, /*includeGlobals*/ false))) {
+                              res.set(text, makeUniqueName(`_${text}`, identifiers));

Member

amcasey Nov 13, 2017

Isn't it redundant to prepend an underscore?

Author

ghost Nov 14, 2017

text is a keyword name; it doesn't appear in the identifiers list, but still needs the underscore to avoid being a keyword. And then if there happens to be an identifier _class we need to change to __class.


          Code review

22c1f76

mhegazy reviewed

View reviewed changes

src/compiler/checker.ts Outdated

@@ @@ -920,6 +921,7 @@ namespace ts { @@
                           nameNotFoundMessage: DiagnosticMessage,
                           nameArg: __String | Identifier,
                           isUse: boolean,
+                          includeGlobals: boolean,

Contributor

mhegazy Nov 30, 2017

seems wierd to have to opt-in resolving a global. i would expect this be the opposite, that you opt out-in to skip globals

mhegazy reviewed

View reviewed changes

tests/cases/fourslash/refactorConvertToEs6Module_export_moduleDotExports.ts Outdated

+              export default 0;
+              export * from "./b";
+              export { default } from "./b";`,

Contributor

mhegazy Nov 30, 2017 •

edited

Loading

this seems wrong to me.. or at least having both exports seem wrong.. i think you either want one or the other.. and to a big extend it depends on what the shape of "./b" is. if "./b" does not have a default export, this statement is an error under ES6 semantics.

I would say we should do one or the other, i.e. always do export * from "./b" unless ",/b" has only one export that happens to be default (or export = for that matter).

mhegazy reviewed

View reviewed changes

tests/cases/fourslash/refactorConvertToEs6Module_import_multipleVariableDeclarations.ts

+                  actionDescription: "Convert to ES6 module",
+                  newContent: `import x from "x";
+              const y = 0;
+              import { z } from "z";`,

Contributor

mhegazy Nov 30, 2017

not sure i understand why this is { z } an not import _z from "z"; const {z} = _z;

Author

ghost Nov 30, 2017

If we had this:

const zNamespace = require("z");
zNamespace.z();

We would translate to this:

import { z } from "z";
z();

right?
Using a destructure in commonjs (const { z } = require("z");) behaves equivalently to the first example so I think we should upgrade it to es6 too.

Contributor

mhegazy Dec 1, 2017

well it depends on what "z" is. if it is ES6 module, then it is fine. if it a cjs file, then it shoudl be import zNamspace from "z"; zNamespace.z().

2 questions:

what if "z" is another file in the project, and the user is going to run the refactoring on "z" next, should we assume it is a default? or that it is a namespace?
What do we do in absence of information, e.g. if this is a module from node_modules, or one we have not seen before.. i would say it safe to assume that everything is cjs unless proven otherwise..

I think the important part here is for us to be consistent between import and export. either everything is a default, or everything is a namespace. let's grab @DanielRosenwasser tomorrow and talk it out.

Author

ghost Dec 5, 2017

Talked with @DanielRosenwasser and I think it's fine to use a named import regardless of the thing we're importing from. @DanielRosenwasser mentioned that we could also choose to provide a second codefix that would always use default imports. But I think we should wait and see if anyone actually wants that, because I think most people will want named imports.

mhegazy reviewed

View reviewed changes

tests/cases/fourslash/refactorConvertToEs6Module_import_nameFromModuleSpecifier.ts

+              const [] = aB;
+              import A from "0a";
+              const [] = A;
+              import _A from "1a";

Contributor

mhegazy Nov 30, 2017

ideally we would have all the import statements at the top and not interleaved with the statements

Author

ghost Nov 30, 2017

Similar comment in #19916 (comment)
I don't think that would be easy to do as we are replacing all of the ... = require(...) statements in place -- we're not guaranteed that these will all be in a row at the top of the file anyway.
This case should be pretty rare though since it's unusual to destructure a module into an array.

mhegazy reviewed

View reviewed changes

tests/cases/fourslash/refactorConvertToEs6Module_import_objectBindingPattern_plain.ts

+                  refactorName: "Convert to ES6 module",
+                  actionName: "Convert to ES6 module",
+                  actionDescription: "Convert to ES6 module",
+                  newContent: 'import { x, y as z } from "x";',

Contributor

mhegazy Nov 30, 2017

same as my earlier comment, i thought this would be a default import.

mhegazy reviewed

View reviewed changes

tests/cases/fourslash/refactorConvertToEs6Module_import_onlyNamedImports.ts

+                  refactorName: "Convert to ES6 module",
+                  actionName: "Convert to ES6 module",
+                  actionDescription: "Convert to ES6 module",
+                  newContent: `import { y } from "x";

Contributor

mhegazy Nov 30, 2017

and this.

Andy Hanson added 3 commits

November 30, 2017 14:47


          includeGlobals -> excludeGlobals

fe19adf


          Improve handling of module.exports = require("...")

c02ddfc


          Merge branch 'master' into convertToEsModule

051c9c8

ghost force-pushed the convertToEsModule branch from fb282fb to 051c9c8 Compare

December 1, 2017 00:08

DanielRosenwasser requested changes

View reviewed changes

src/compiler/factory.ts

@@ @@ -101,7 +101,7 @@ namespace ts { @@
                       return node;
                   }
-                  function createLiteralFromNode(sourceNode: StringLiteral | NumericLiteral | Identifier): StringLiteral {
+                  function createLiteralFromNode(sourceNode: StringLiteralLike | NumericLiteral | Identifier): StringLiteral {

Member

DanielRosenwasser Dec 4, 2017

This isn't internal, but it uses StringLiteralLike. You can technically make an internal overload for it, or use NoSubstitutionTemplateLiteral

Author

ghost Dec 4, 2017 •

edited by ghost

Loading

This isn't exported so it won't show up in the public API. API baseline tests would have told us if we tried to use an internal type in a public api.


          Allow NoSubstitutionTemplateLiteral as argument to createLiteral

4df53c1

ghost force-pushed the convertToEsModule branch from 4989ae5 to 4df53c1 Compare

December 5, 2017 20:53

Author

ghost commented Dec 6, 2017

@DanielRosenwasser @mhegazy Good to go?

mhegazy approved these changes

View reviewed changes

Contributor

mhegazy commented Jan 9, 2018

@andy-ms can you please refresh and merge.

ghost force-pushed the convertToEsModule branch from 540706e to 53c9bf2 Compare

January 9, 2018 20:54


          Merge branch 'master' into convertToEsModule

d67ef00

ghost force-pushed the convertToEsModule branch from 53c9bf2 to d67ef00 Compare

January 9, 2018 21:01

ghost merged commit 8bce69e into master

ghost deleted the convertToEsModule branch

January 9, 2018 21:15

ghost mentioned this pull request

Common import/export refactorings are missing #19260

Closed

microsoft locked and limited conversation to collaborators

This pull request was closed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet