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

Add babel-plugin-syntax-typescript, babel-plugin-transform-typescript, and babel-preset-typescript #5899

Merged
merged 11 commits into from Aug 7, 2017

Conversation

ghost
Copy link

@ghost ghost commented Jun 28, 2017

Q A
Patch: Bug Fix? no
Major: Breaking Change? no
Minor: New Feature? yes
Deprecations? no
Spec Compliancy? no
Tests Added/Pass? tests added and pass
Fixed Tickets
License MIT
Doc PR
Dependency Changes no

Adds babel-plugin-syntax-typescript, which just enables the "typescript" babylon plugin.
Adds babe-plugin-transform-typescript, which transforms TypeScript-specific features to ES.Next.
Note: This contains a TODO to be removed once #5856 is in.
This does not include the full test suite, which will require this, #5856, and #5896 to all come together.

@mention-bot
Copy link

@andy-ms, thanks for your PR! By analyzing the history of the files in this pull request, we identified @hzoo, @existentialism and @loganfsmyth to be potential reviewers.

@hzoo
Copy link
Member

hzoo commented Jun 28, 2017

We can do babel-preset-typescript here too although it would just be the plugin

@hzoo hzoo added area: typescript PR: New Feature 🚀 A type of pull request used for our changelog categories labels Jun 28, 2017
@ghost ghost changed the title Add babel-plugin-syntax-typescript and babel-plugin-transform-typescript Add babel-plugin-syntax-typescript, babel-plugin-transform-typescript, and babel-preset-typescript Jun 28, 2017
}

// Based on the TypeScript repository's `evalConstant` in `checker.ts`.
function evaluate(expr, seen: PreviousEnumMembers) {
Copy link
Member

Choose a reason for hiding this comment

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

cool, you probably want to just use the same logic but just FYI if you didn't know we have a path.evaluate as well

Copy link
Author

Choose a reason for hiding this comment

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

Also, we need to be able to evaluate previously-seen enum values, so any function on just the path wouldn't be enough:

enum E {
  A = 1,
  B = A * 2,
  C, // inferred as 3
}

Copy link
Member

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

I'll review the enum stuff later.

Program(path, state: State) {
state.sourceFileHasJsx = false;
path.traverse({
JSXOpeningElement() {
Copy link
Member

Choose a reason for hiding this comment

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

This shouldn't be necessary.


Program(path, state: State) {
state.sourceFileHasJsx = false;
path.traverse({
Copy link
Member

Choose a reason for hiding this comment

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

We can avoid a full file traversal if we only do this when we encounter a import React.


const assigns = parameterProperties.map(p => {
let name;
if (p.type === "Identifier") {
Copy link
Member

Choose a reason for hiding this comment

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

t.isIdentifier(p). Doing explicit comparisons against node.type is an anti-pattern.

);
}

const _this = t.thisExpression();
Copy link
Member

Choose a reason for hiding this comment

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

You could just inline this.


const _this = t.thisExpression();
const l = t.memberExpression(_this, t.identifier(name));
const ass = t.assignmentExpression("=", l, t.identifier(name));
Copy link
Member

Choose a reason for hiding this comment

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

Lol. 👦

? [statements[0], ...assigns, ...statements.slice(1)]
: [...assigns, ...statements];

function isSuperCall(x: Statement | typeof undefined): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

Hoist please.

if (node.returnType) node.returnType = null;

const p0 = node.params[0];
if (p0 && p0.type === "Identifier" && p0.name === "this") {
Copy link
Member

Choose a reason for hiding this comment

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

t.isIdentifier(p0, {name: "this"})


const p0 = node.params[0];
if (p0 && p0.type === "Identifier" && p0.name === "this") {
node.params = node.params.slice(1);
Copy link
Member

Choose a reason for hiding this comment

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

Why not just shift it?

t.identifier("exports"),
);
path.replaceWith(
t.assignmentExpression("=", moduleExports, path.node.expression),
Copy link
Member

Choose a reason for hiding this comment

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

Should we be generating an export statement instead, so they can transform into whatever their bundler expects?

Copy link
Author

Choose a reason for hiding this comment

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

Actually, it doesn't make a lot of sense to support this feature in babel, especially since we don't have support for import = either. Removed.

@@ -32,7 +32,7 @@
"babel-template": "7.0.0-alpha.12",
"babel-traverse": "7.0.0-alpha.12",
"babel-types": "7.0.0-alpha.12",
"babylon": "7.0.0-beta.15",
"babylon": "7.0.0-beta.16",
Copy link
Member

Choose a reason for hiding this comment

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

This is set to beta.18 in 7.0 now so can remove these (or just update to 18)

This preset includes the following plugins:

- [transform-typescript](https://babeljs.io/docs/plugins/transform-typescript/)
- [syntax-object-rest-spread](https://babeljs.io/docs/plugins/syntax-object-rest-spread/)
Copy link
Member

@hzoo hzoo Jul 28, 2017

Choose a reason for hiding this comment

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

If we add this, should we add class properties too? Not sure

Copy link
Author

@ghost ghost Jul 28, 2017

Choose a reason for hiding this comment

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

We just want to enable syntax, not add any transforms. (The syntax should already be enabled via babel-plugin-syntax-typescript.) That way people can have an emit that includes class properties (minus types) if they want to.

Copy link
Member

Choose a reason for hiding this comment

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

oh ok I would of assumed object rest spread was enabled by syntax too

Copy link
Member

@hzoo hzoo Aug 7, 2017

Choose a reason for hiding this comment

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

Ok I thought that either babel-plugin-syntax-typescript would just include it there rather than putting it in the preset https://github.com/babel/babel/pull/5899/files#diff-36ceea99653860f193c748d23736fe0aR4

or that babylon would have a similar || check for object spread https://github.com/babel/babylon/blob/c88af90c0a88ec756ad7c27f0a1c6f92bffbcd24/src/parser/statement.js#L1148 - just didn't seem consistent for both class properties and object spread.

Just didn't seem to be necessary to put it in the preset if it can go in the syntax plugin or in babylon; can handle in a followup PR though

@@ -136,7 +136,11 @@ export function verify(visitor) {
if (shouldIgnoreKey(nodeType)) continue;

if (t.TYPES.indexOf(nodeType) < 0) {
throw new Error(messages.get("traverseVerifyNodeType", nodeType));
if (
!nodeType.startsWith("TS") // TODO: https://github.com/babel/babel/pull/5856
Copy link
Member

Choose a reason for hiding this comment

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

you already know since this is merged, + merge conflict

@hzoo hzoo added this to the Babel 7 milestone Aug 4, 2017
}
},

TSDeclareFunction(path) {
Copy link
Member

Choose a reason for hiding this comment

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

Probably not critical, but Flow types have virtual aliases (i.e. Flow, etc), we may want to do the same here and it would let you replace a bunch of these visitors with:

Typescript(path) {
  path.remove();
}

Copy link
Member

Choose a reason for hiding this comment

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

Can do this in the follow up PR

Copy link
Author

@ghost ghost Aug 8, 2017

Choose a reason for hiding this comment

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

The only ones which can be directly removed are TSDeclareFunction, TSDeclareMethod, TSIndexSignature, TSInterfaceDeclaration, and TSTypeAliasDeclaration.
What these all have in common is that they are statements, and are always just declarations (no emit like an enum has).

Nodes like TSAsExpression can't be entirely removed (they contain expressions), and nodes like TSModuleDeclaration require special handling depending on whether they're a type declaration or have a value side too.

If we just added a TypeScript alias it would be confusing why that doesn't include anything else. I could make a TSNonValueDeclaration alias though?

@hzoo hzoo merged commit e37a5eb into babel:7.0 Aug 7, 2017
@hzoo
Copy link
Member

hzoo commented Aug 7, 2017

Whoo, thanks @andy-ms!! Time to test it 😄 @DanielRosenwasser

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: typescript outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: New Feature 🚀 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants