GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Add refactoring to convert CommonJS module to ES6 module · Pull Request #19916 · microsoft/TypeScript

Conversation

@ghost

@ghost Deleted user (ghost) commented Nov 10, 2017

Copy link
Copy Markdown

Fixes #19719

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ghost
ghost requested a review from Andrew Casey (amcasey) November 10, 2017 17:44
@ghost
ghost force-pushed the convertToEsModule branch 2 times, most recently from 4118c0c to 517715a Compare November 10, 2017 18:58
Andy Hanson
Add refactoring to convert CommonJS module to ES6 module
a4b588d
@ghost
ghost force-pushed the convertToEsModule branch from 517715a to a4b588d Compare November 10, 2017 19:00
@amcasey

Andrew Casey (amcasey) commented Nov 13, 2017

Copy link
Copy Markdown
Member

Someone who understands modules should probably take a look too.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ghost

Deleted user (ghost) commented Nov 13, 2017

Copy link
Copy Markdown
Author

Ben Lichtman (@uniqueiniquity) Mine Starks (@minestarks) Armando Aguirre (@armanio123) Arthur Ozga (@aozgaa) Someone want to take this on?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
amcasey
Andrew Casey (amcasey) reviewed Nov 13, 2017
View reviewed changes

@amcasey Andrew Casey (amcasey) left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Notes on all files but convertToEs6Module.ts.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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.

@amcasey Andrew Casey (amcasey) Nov 13, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Copy-paste error?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread tests/cases/fourslash/refactorConvertToEs6Module_export_referenced.ts

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

@amcasey Andrew Casey (amcasey) Nov 13, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ghost Deleted user (ghost) Nov 14, 2017

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread tests/cases/fourslash/refactorConvertToEs6Module_import_multipleUniqueIdentifiers.ts
newContent: `import x from "x";
import _x from "x";
const [a, b] = _x;
import __x from "x";

@amcasey Andrew Casey (amcasey) Nov 13, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Are imports supposed to go at the top?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ghost Deleted user (ghost) Nov 14, 2017

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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'`,

@amcasey Andrew Casey (amcasey) Nov 13, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/core.ts Outdated
return result;
}

export function mapIter<T, U>(iter: Iterator<T>, mapFn: (x: T) => U): U[] {

@amcasey Andrew Casey (amcasey) Nov 13, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/core.ts
return result || array;
}

export function mapAllOrFail<T, U>(array: ReadonlyArray<T>, mapFn: (x: T, i: number) => U | undefined): U[] | undefined {

@amcasey Andrew Casey (amcasey) Nov 13, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ghost Deleted user (ghost) Nov 14, 2017

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/compiler/core.ts Outdated
const result: U[] = [];
for (let i = 0; i < array.length; i++) {
const mapped = mapFn(array[i], i);
if (!mapped) {

@amcasey Andrew Casey (amcasey) Nov 13, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
amcasey
Andrew Casey (amcasey) approved these changes Nov 13, 2017
View reviewed changes

@amcasey Andrew Casey (amcasey) left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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:

@amcasey Andrew Casey (amcasey) Nov 13, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Typo "conver to"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/services/refactors/convertToEs6Module.ts Outdated
});
}

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

@amcasey Andrew Casey (amcasey) Nov 13, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

isAssignment means that node is the LHS of an assignment?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/services/refactors/convertToEs6Module.ts
}

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

@amcasey Andrew Casey (amcasey) Nov 13, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Out of curiosity, why alias boolean like this?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ghost Deleted user (ghost) Nov 14, 2017

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
kant2002 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread 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 });

@amcasey Andrew Casey (amcasey) Nov 13, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think the refactoring context might contain a newline character.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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`.

@amcasey Andrew Casey (amcasey) Nov 13, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ghost Deleted user (ghost) Nov 14, 2017 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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"));

@amcasey Andrew Casey (amcasey) Nov 13, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ghost Deleted user (ghost) Nov 14, 2017

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/services/refactors/convertToEs6Module.ts
case SyntaxKind.ClassExpression:
// `exports.C = class {}` --> `export class C {}`
return classExpressionToDeclaration(name, modifiers, exported as ClassExpression);
default:

@amcasey Andrew Casey (amcasey) Nov 13, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ghost Deleted user (ghost) Nov 14, 2017

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/services/refactors/convertToEs6Module.ts
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));

@amcasey Andrew Casey (amcasey) Nov 13, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Isn't it redundant to prepend an underscore?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ghost Deleted user (ghost) Nov 14, 2017

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Andy Hanson
Code review
22c1f76
mhegazy
Mohamed Hegazy (mhegazy) reviewed Nov 30, 2017
View reviewed changes
Comment thread src/compiler/checker.ts Outdated
nameNotFoundMessage: DiagnosticMessage,
nameArg: __String | Identifier,
isUse: boolean,
includeGlobals: boolean,

@mhegazy Mohamed Hegazy (mhegazy) Nov 30, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mhegazy
Mohamed Hegazy (mhegazy) reviewed Nov 30, 2017
View reviewed changes
Comment thread tests/cases/fourslash/refactorConvertToEs6Module_export_moduleDotExports.ts Outdated
export default 0;

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

@mhegazy Mohamed Hegazy (mhegazy) Nov 30, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mhegazy
Mohamed Hegazy (mhegazy) reviewed Nov 30, 2017
View reviewed changes
Comment thread tests/cases/fourslash/refactorConvertToEs6Module_import_multipleVariableDeclarations.ts
actionDescription: "Convert to ES6 module",
newContent: `import x from "x";
const y = 0;
import { z } from "z";`,

@mhegazy Mohamed Hegazy (mhegazy) Nov 30, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ghost Deleted user (ghost) Nov 30, 2017

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mhegazy Mohamed Hegazy (mhegazy) Dec 1, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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:

  1. 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?
  2. 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 Daniel Rosenwasser (@DanielRosenwasser) tomorrow and talk it out.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ghost Deleted user (ghost) Dec 5, 2017

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Talked with Daniel Rosenwasser (@DanielRosenwasser) and I think it's fine to use a named import regardless of the thing we're importing from. Daniel Rosenwasser (@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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mhegazy
Mohamed Hegazy (mhegazy) reviewed Nov 30, 2017
View reviewed changes
Comment thread tests/cases/fourslash/refactorConvertToEs6Module_import_nameFromModuleSpecifier.ts
const [] = aB;
import A from "0a";
const [] = A;
import _A from "1a";

@mhegazy Mohamed Hegazy (mhegazy) Nov 30, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ghost Deleted user (ghost) Nov 30, 2017

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mhegazy
Mohamed Hegazy (mhegazy) reviewed Nov 30, 2017
View reviewed changes
Comment thread 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";',

@mhegazy Mohamed Hegazy (mhegazy) Nov 30, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mhegazy
Mohamed Hegazy (mhegazy) reviewed Nov 30, 2017
View reviewed changes
Comment thread 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";

@mhegazy Mohamed Hegazy (mhegazy) Nov 30, 2017

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

and this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Andy Hanson added 3 commits November 30, 2017 14:47
Andy Hanson
includeGlobals -> excludeGlobals
fe19adf
Andy Hanson
Improve handling of module.exports = require("...")
c02ddfc
Andy Hanson
Merge branch 'master' into convertToEsModule
051c9c8
@ghost
ghost force-pushed the convertToEsModule branch from fb282fb to 051c9c8 Compare December 1, 2017 00:08
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) requested changes Dec 4, 2017
View reviewed changes
Comment thread src/compiler/factory.ts
}

function createLiteralFromNode(sourceNode: StringLiteral | NumericLiteral | Identifier): StringLiteral {
function createLiteralFromNode(sourceNode: StringLiteralLike | NumericLiteral | Identifier): StringLiteral {

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Dec 4, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ghost Deleted user (ghost) Dec 4, 2017 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Andy Hanson
Allow NoSubstitutionTemplateLiteral as argument to createLiteral
4df53c1
@ghost
ghost force-pushed the convertToEsModule branch from 4989ae5 to 4df53c1 Compare December 5, 2017 20:53
@ghost

Deleted user (ghost) commented Dec 6, 2017

Copy link
Copy Markdown
Author

Daniel Rosenwasser (@DanielRosenwasser) Mohamed Hegazy (@mhegazy) Good to go?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mhegazy
Mohamed Hegazy (mhegazy) approved these changes Jan 9, 2018
View reviewed changes
@mhegazy

Mohamed Hegazy (mhegazy) commented Jan 9, 2018

Copy link
Copy Markdown
Contributor

Andy (Andrewkraft) (@Andy-MS) can you please refresh and merge.
styfle reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ghost
ghost force-pushed the convertToEsModule branch from 540706e to 53c9bf2 Compare January 9, 2018 20:54
Andy Hanson
Merge branch 'master' into convertToEsModule
d67ef00
@ghost
ghost force-pushed the convertToEsModule branch from 53c9bf2 to d67ef00 Compare January 9, 2018 21:01
@ghost
ghost merged commit 8bce69e into master Jan 9, 2018
@ghost
ghost deleted the convertToEsModule branch January 9, 2018 21:15
@ghost ghost mentioned this pull request Jan 11, 2018
Common import/export refactorings are missing #19260
Closed
@microsoft Microsoft (microsoft) locked and limited conversation to collaborators Jul 3, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@amcasey @mhegazy @DanielRosenwasser
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.