Babel 6 types by hzoo · Pull Request #162 · benjamn/ast-types

hzoo · 2016-07-04T16:36:28Z

Fixes #132
For babel pr: babel/babel#3561
Also ref benjamn/recast#298

First 3 commits are from #145 (fixed merge conflicts by preferring the changes in the PR).

You can review that PR or this one with whitespace off for a better diff https://github.com/benjamn/ast-types/pull/162/files?w=1.

My commit adds the babel 6 types (not sure if it's right though, so please review!).

References:

babel-types definitions: https://github.com/babel/babel/tree/master/packages/babel-types/src/definitions
babylon ast differences https://github.com/babel/babylon#output

cc @jamestalmage @benjamn

hzoo · 2016-07-04T16:38:38Z

def/babel6.js

@@ -0,0 +1,117 @@
+module.exports = function (fork) {
+    fork.use(require("./babel"));
+    fork.use(require("./flow"));


Do I need to fork both? or should babel fork flow instead of es7?

I think you should fork both. It's conceivable people will want to use the types defined in ./babel without those defined in ./flow, right? If ./babel forks ./flow, it should imply that those type definitions somehow depend on ./flow.

hzoo · 2016-07-04T16:41:04Z

General question - is it better to just define a new type with everything from scratch (base, build, field)? Some of the changes are literally just a name change so that probably doesn't have to be.

hzoo · 2016-07-04T21:05:48Z

Should I be modifying main.js?

jamestalmage · 2016-07-06T01:58:24Z

fixed merge conflicts by preferring the changes in the PR

That was so long ago, there are probably changes that should have been merged more carefully. I can take a look at #145 again tonight.

jamestalmage · 2016-07-06T15:22:36Z

General question - is it better to just define a new type with everything from scratch (base, build, field)? Some of the changes are literally just a name change so that probably doesn't have to be.

My gut says just redefine. Babel has moved far enough away from esprima compatibility, that attempting to share base types seems likely to create some amount of pain, and I don't really see much of an upside. We are to the point of needing a custom fork anyways, so might as well just make the entire tree conform to the Babel way of doing things.

That said, simple name changes can probably just be accomplished with:

def('newName').bases('oldName')

(you might need to redefine build in that case, not sure)

hzoo · 2016-07-07T14:57:41Z

Rebased again

benjamn · 2016-07-11T20:12:26Z

def/babel6.js

    // Split Literal
    def("StringLiteral")
-        .bases("Literal")
+        .bases("Node", "Expression")


Node is not important here, nor for the other Babel *Literal types, because Expression <: Node.

benjamn · 2016-07-11T20:29:57Z

Can you rebase now that Make forkable #145 has been merged?
I believe you can solve the test failures by creating multiple forks for different tests, instead of using the default fork that includes all the defs.

mroch · 2016-10-02T02:46:01Z

def/babel6.js

+    def("ObjectPattern")
+        .bases("Pattern")
+        .build("properties")
+        .field("properties", [or(def("RestProperty"), def("ObjectProperty"))])


it should be Property, not ObjectProperty, per https://github.com/estree/estree/blob/f32646033ed12e6aaa7f5cd496d936a8f8de194e/es2015.md#objectpattern

There are a couple of places in ESTree where nodes have different type values like this, but I don't think ast-types has a way to represent this.

What do you mean here though? I'm defining all the different types from ESTree (including ObjectProperty above) - I thought we would be forking to support the different AST types in babel (6).

https://github.com/babel/babylon#output

Note how ObjectPattern.properties is an array of ObjectProperty's, like you have, but ObjectProperty's type field is "Property", not "ObjectProperty." def("ObjectProperty") requires an ast with { type: "ObjectProperty" }, but estree says it should be { type: "Property" }

After v6, Babel isn't following ESTree completely so I had to make this PR to make it work with recast. Yeah it's totally correct it should be Property but Babylon won't produce a type of Property anymore so I'm using ObjectProperty if that makes sense (unless I'm misunderstanding something)

suchipi · 2016-10-04T01:05:06Z

I'd like to see this merged... how can I contribute? What needs to be done to fix the tests? I'm not familiar enough with the forking model in this codebase to understand how to fix failures.

hzoo · 2016-11-04T12:36:19Z

Looked through https://github.com/babel/babel/commits/master/packages/babel-generator to find new syntax.

hzoo · 2016-11-04T12:36:37Z

def/babel6.js

+      .build("argument")
+      .field("argument", def("Expression"));
+
+    def("ForAwaitStatement")


babel/babel@26e79c5

hzoo · 2016-11-04T12:36:50Z

def/flow.js

      .bases("Type")
      .build();

+    def("EmptyTypeAnnotation")


babel/babel#4724

hzoo · 2016-11-04T12:37:19Z

def/flow.js

      .field("value", def("Type"))
-      .field("optional", Boolean);
+      .field("optional", Boolean)
+      .field("variance",


babel/babel#4697

hzoo · 2016-11-04T12:37:29Z

def/flow.js

      .field("id", or(def("Identifier"), def("Literal")))
      .field("body", def("BlockStatement"));

+    def("DeclareModuleExports")


babel/babel#3570

hzoo · 2016-11-04T12:58:39Z

Finally got around to fixing these.. @suchipi (I didn't understand the forking either).

Tested with benjamn/recast#299 (would error that node isn't printable, etc)

hzoo · 2016-11-04T14:53:52Z

Thanks @benjamn!!! much excite 😍

hzoo reviewed Jul 4, 2016
View reviewed changes

hzoo changed the title ~~Jamestalmage make forkable~~ Babel 6 types + forkable Jul 4, 2016

hzoo mentioned this pull request Jul 4, 2016

babel-core: add options for different parser/generator babel/babel#3561

Merged

3 tasks

jamestalmage mentioned this pull request Jul 6, 2016

Make forkable #145

Merged

benjamn reviewed Jul 11, 2016
View reviewed changes

hzoo mentioned this pull request Jul 15, 2016

Rewriting Lebab as a Babel transformer? lebab/lebab#138

Open

hzoo mentioned this pull request Aug 31, 2016

Support for new babel types #132

Closed

hzoo mentioned this pull request Sep 17, 2016

Not usable with babylon parser's custom nodes facebook/jscodeshift#149

Closed

mroch reviewed Oct 2, 2016

View reviewed changes

hzoo changed the title ~~Babel 6 types + forkable~~ Babel 6 types Nov 3, 2016

hzoo commented Nov 4, 2016

View reviewed changes

def/flow.js

.bases("Type")

.build();

def("EmptyTypeAnnotation")

Copy link
Copy Markdown

Contributor Author

hzoo Nov 4, 2016

Choose a reason for hiding this comment

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

babel/babel#4724

hzoo commented Nov 4, 2016

View reviewed changes

hzoo mentioned this pull request Nov 4, 2016

Babel 6 changes + flow benjamn/recast#299

Merged

babel 6 node types

4c7b343

benjamn merged commit d173232 into benjamn:master Nov 4, 2016

hzoo mentioned this pull request Nov 4, 2016

Add ForAwaitStatement #187

Closed

Conversation

hzoo commented Jul 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hzoo commented Jul 4, 2016

Uh oh!

hzoo commented Jul 4, 2016

Uh oh!

jamestalmage commented Jul 6, 2016

Uh oh!

jamestalmage commented Jul 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hzoo commented Jul 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjamn commented Jul 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suchipi commented Oct 4, 2016

Uh oh!

hzoo commented Nov 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hzoo commented Nov 4, 2016

Uh oh!

hzoo commented Nov 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hzoo commented Jul 4, 2016 •

edited

Loading

jamestalmage commented Jul 6, 2016 •

edited

Loading