Rewrite Hub as interface #5047 #5050

yongxu · 2016-12-28T01:40:34Z

Q	A
Patch: Bug Fix?	yes
Major: Breaking Change?	no
Minor: New Feature?	yes
Deprecations?	no
Spec Compliancy?	yes
Tests Added/Pass?	yes
Fixed Tickets	Fixes #5047, Fixes #4640, Fixes #4413
License	MIT
Doc PR	no
Dependency Changes	no

This PR implements Rewrite Hub as interface #5047 <--check out more details in this issue
will make babel-traverse a standalone package

codecov-io · 2016-12-28T01:50:26Z

Codecov Report

❗ No coverage uploaded for pull request base (master@283d9cb).

@@            Coverage Diff            @@
##             master    #5050   +/-   ##
=========================================
  Coverage          ?   89.22%           
=========================================
  Files             ?      203           
  Lines             ?     9848           
  Branches          ?     2628           
=========================================
  Hits              ?     8787           
  Misses            ?     1061           
  Partials          ?        0

Impacted Files	Coverage Δ
packages/babel-traverse/src/path/index.js	`88.65% <100%> (ø)`
packages/babel-traverse/src/hub.js	`100% <100%> (ø)`
packages/babel-traverse/src/scope/index.js	`84.63% <100%> (ø)`
packages/babel-traverse/src/index.js	`93.47% <100%> (ø)`
packages/babel-traverse/src/path/introspection.js	`57.95% <75%> (ø)`
...ckages/babel-core/src/transformation/file/index.js	`90.42% <80%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 283d9cb...7f1e8dc. Read the comment docs.

xtuc · 2016-12-30T07:57:20Z

packages/babel-traverse/src/index.js

@@ -9,7 +9,8 @@ import * as cache from "./cache";

 export { default as NodePath } from "./path";
 export { default as Scope } from "./scope";
-export { default as Hub } from "./hub";
+import type Hub from "./hub";
+export type { Hub };


I guess there isn't an export shortcut as in ES6 ? Like:

export type Hub from "./hub";

no it isn't, this is how flow export types. this has no influence to the compiled code.

xtuc · 2016-12-30T07:58:35Z

packages/babel-traverse/src/path/index.js

-      message,
-      loc: this.node.loc
-    });
+    this.hub.mark && this.hub.mark(type, message, this.node.loc);


To be consistent in coding style, could you use an explicit if there?

xtuc · 2016-12-30T08:01:51Z

packages/babel-traverse/src/hub.js

+  addHelper: (name: string) => Object;
+  getScope: () => Object;
+  getCode: () => string;
+  buildError:(node: Object, msg: string, Error: Error) => Error;


I don't know if some users rely on this class.

I would like to keep the class there, what do you think? I'm not sure because the class is empty anyway.

yeah, totally! I think using export default class Hub instead export type Hub would probably be good here.
Though Hub itself wasn't been referred in repo, it is possible that someone used Hub to store states. We can totally keep it.

xtuc · 2016-12-30T08:02:15Z

packages/babel-plugin-transform-react-jsx-source/src/index.js

@@ -50,7 +50,8 @@ export default function ({ types: t }) {
          : null;

        const fileNameIdentifier = path.scope.generateUidIdentifier(FILE_NAME_VAR);
-        path.hub.file.scope.push({id: fileNameIdentifier, init: t.stringLiteral(fileName)});
+        const scope = path.hub.getScope();
+        scope && scope.push({id: fileNameIdentifier, init: t.stringLiteral(fileName)});


Could you use an explicit if there?

absolutely! Thanks for reviewing. Working on it :)

xtuc · 2016-12-30T08:07:00Z

packages/babel-traverse/src/hub.js

+export type Hub = {
+  mark: (type: string, message: string) => void;
+  addHelper: (name: string) => Object;
+  getScope: () => Object;


I think getScope returns a Scope object.

yongxu · 2016-12-31T22:17:21Z

@xtuc what do you think about this new version? I made some changes based on your suggestion.
happy new year btw 👍

xtuc · 2017-01-01T13:04:10Z

packages/babel-core/src/transformation/file/index.js

+      getCode: () => this.code,
+      getScope: () => this.scope,
+      buildError: this.buildCodeFrameError.bind(this)
+    };


I thought you gonna replace the object with the Hub class in packages/babel-traverse/src/hub.js. Can we replace it?

Like:

hub: HubInterface; this.hub = new Hub(this);

xtuc · 2017-01-01T13:09:56Z

packages/babel-traverse/src/hub.js

-  constructor(file, options) {
-    this.file = file;
-    this.options = options;
+  buildError(node, msg, Error): Error {


The coverage fails there because the method buildError isn't tested.

If you move the content of the object (as described above) there, we need to add tests for it.

I added a test for this, though the test doesn't seem very necessary.
It did pass the coverage test now! Thanks for reviewing :)

yongxu · 2017-01-08T10:05:50Z

@xtuc any progress or request on this PR? Thanks

loganfsmyth · 2017-01-14T19:41:42Z

packages/babel-traverse/src/hub.js

-    this.file = file;
-    this.options = options;
+  buildError(node, msg, Error): Error {
+    return new TypeError(msg);


This should be new Error since this function should return a type based on the third parameter of the function.

loganfsmyth · 2017-01-14T19:43:17Z

packages/babel-core/src/transformation/file/index.js

@@ -99,7 +100,19 @@ export default class File extends Store {
    this.code    = "";
    this.shebang = "";

-    this.hub = new Hub(this);
+    this.hub = {


I know you removed the file property from the interface, but we'll definitely need to keep it for the usage in babel-core at least, or else it will break people's plugins that read properties off of hub. For example some plugins do path.hub.file.opts.filename to get the name of the file being compiled.

nice catch! fixed it :)

yongxu · 2017-01-17T20:06:32Z

packages/babel-traverse/src/hub.js

-    this.file = file;
-    this.options = options;
+  buildError(node, msg, BuildError = TypeError): Error {
+    return new BuildError(msg);


@loganfsmyth what do you think about this approach? when Error is not specified, use TypeError as defaiult.
also changed its name to BuildError in the implementation, which is more descriptive IMO.

babel-bot · 2017-01-31T17:25:59Z

Hey @yongxu! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

Node vnode - linux

hzoo · 2017-02-13T20:55:34Z

Whoo thanks @yongxu 🎉, sorry for the delay!! 🙏

xtuc · 2017-02-13T20:57:13Z

Nice work 👍

yongxu · 2017-02-13T20:59:30Z

Thanks guys! Appreciate it, will contribute more :)

loganfsmyth · 2017-02-14T02:22:44Z

Heya @yongxu, unfortunately I missed in review that this would be a breaking change, so we've had to revert it in #5306 due to reports of errors in #5305.

yongxu · 2017-02-14T05:45:10Z

I am really sorry about this breaking issue @loganfsmyth , I am looking into it.

* Add new flow preset (#5288) * Fix PathHoister hoisting JSX member expressions on "this". (#5143) The PathHoister ignored member references on "this", causing it to potentially hoist an expression above its function scope. This patch tells the hoister to watch for "this", and if seen, mark the nearest non-arrow function scope as the upper limit for hoistng. This fixes #4397 and is an alternative to #4787. * Fix PathHoister hoisting before bindings. (#5153) Fixes #5149 and enables a few additional safe hoists. * Fix linting error * feature: Support pure expressions in transform-react-constant-elements (#4812) * Fix loose for-of with label (#5298) * Rewrite Hub as interface #5047 (#5050) * Rewrite Hub as interface #5047 * Update index.js * Avoid adding unnecessary closure for block scoping (#5246) When you write ``` for (const x of l) { setTimeout(() => x); } ``` we need to add a closure because the variable is meant to be block-scoped and recreated each time the block runs. We do this. However, we also add the closure when no loop is present. This isn't necessary, because if no loop is present then each piece of code runs at most once. I changed the transform to only add a closure if a variable is referenced from within a loop. * Add greenkeeperio-bot to mention-bot blacklist (#5301) [skip ci] * Upgrade lerna to current beta. (#5300) * Revert "Upgrade lerna to current beta." (#5303) * Add charset so tests work with convert-source-map@>1.4 (#5302) * Add CHANGELOG for 6.23.0 [skip ci] (#5304) * Update babel-types README from script. * v6.23.0 * Revert change that lerna force-committed. * Revert "Rewrite Hub as interface #5047" (#5306) * v6.23.1 * Revert lerna again

loganfsmyth · 2017-02-20T08:36:40Z

@yongxu Let's pull this conversation back over here, since the other thread is about the revert.

The issue that this case is because babel-core is set up to depend on essentially babel-traverse@>6.x. That means that any past version of babel-core will use the most recent version of babel-traverse. That was the core issue here, because the old babel-coreversion would callnew Hub(this)and create aHubinstance with only a.fileproperty and no methods. But the new interface defined bybabel-traverse` in this PR.

babel-traverse's usage of new methods on Hub is a breaking change, since those methods were not required for usage of the old version of babel-traverse.

yongxu · 2017-02-20T22:32:50Z

Yeah that is very true. Now I totally agree with you to leave this out in the current master.
I did see a 7.0 branch however, should I make another PR targeting that branch instead?

loganfsmyth · 2017-02-20T23:15:42Z

Yup! If you'd like to resubmit this for 7.0, I think that would be fine, as long as you are cool with waiting a bit longer for it to be released.

olsonpm · 2017-03-03T07:40:58Z

So I'm trying to use babel-traverse as a standalone library (just traverses an ast built from babylon). I found this pr and merged it into a local clone of babel branch 7.0. The below file links all point to yongxu's fork.

When trying to use path.buildCodeFrameError I get the following error

TypeError: Cannot read property 'buildError' of undefined
    at NodePath.buildCodeFrameError (/.../babel/packages/babel-traverse/lib/path/index.js:174:20)

where that line in source can be found here

buildCodeFrameError(msg: string, Error: typeof Error = SyntaxError): Error {
    return this.hub.buildError(this.node, msg, Error);
}

Looking into it, I don't see where a hub object is created besides in babel-core, which isn't included in babel-traverse. When I debug, sure enough the hub property is always undefined.

What am I missing? I just don't see how this is a standalone package as-is.

yongxu · 2017-03-07T04:10:55Z

@olsonpm If you are using babel-traverse as a standalone and call hub, you'll have to inject hub yourself.

I think it would be great to have a definition for the hub interface, or probably a better name. I'll take a look sometime soon.

This PR was reverted due to minor version resolving issue. Now since we are progressing to v7, should we try to merge it back? @loganfsmyth

Sorry for the delayed response, had been too busy recently. :)

* Rewrite Hub as interface #5047 * Update index.js

yongxu force-pushed the hub branch 2 times, most recently from 78dd051 to e034c4f Compare December 28, 2016 21:52

xtuc reviewed Dec 30, 2016

View reviewed changes

yongxu force-pushed the hub branch 3 times, most recently from de50db4 to 4e14889 Compare December 30, 2016 23:05

xtuc reviewed Jan 1, 2017

View reviewed changes

yongxu force-pushed the hub branch from 4e14889 to c91d74e Compare January 3, 2017 07:13

xtuc self-requested a review January 10, 2017 11:46

loganfsmyth requested changes Jan 14, 2017

View reviewed changes

yongxu force-pushed the hub branch from c91d74e to 0797141 Compare January 17, 2017 19:55

Rewrite Hub as interface babel#5047

9de4399

yongxu force-pushed the hub branch from 0797141 to 9de4399 Compare January 17, 2017 20:01

yongxu commented Jan 17, 2017

View reviewed changes

Merge branch 'master' into hub

0a4d6c0

Update index.js

7f1e8dc

loganfsmyth merged commit 2985597 into babel:master Feb 13, 2017

hzoo added PR: Internal 🏠 A type of pull request used for our changelog categories PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Feb 13, 2017

hzoo mentioned this pull request Feb 14, 2017

Crash caused by newly released babel-transform@66.23.0 #5305

Closed

loganfsmyth mentioned this pull request Feb 14, 2017

Revert "Rewrite Hub as interface #5047" #5306

Merged

existentialism pushed a commit that referenced this pull request May 19, 2017

Rewrite Hub as interface #5047 (#5050)

8a7c954

* Rewrite Hub as interface #5047 * Update index.js

nicolo-ribaudo mentioned this pull request Aug 14, 2018

Rewrite Hub as interface #8469

Merged

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 bot locked as resolved and limited conversation to collaborators Oct 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite Hub as interface #5047 #5050

Rewrite Hub as interface #5047 #5050

yongxu commented Dec 28, 2016 •

edited by xtuc

codecov-io commented Dec 28, 2016 •

edited

xtuc Dec 30, 2016

yongxu Dec 30, 2016

xtuc Dec 30, 2016

xtuc Dec 30, 2016

yongxu Dec 30, 2016

xtuc Dec 30, 2016

yongxu Dec 30, 2016

xtuc Dec 30, 2016 •

edited

yongxu commented Dec 31, 2016

xtuc Jan 1, 2017

xtuc Jan 1, 2017

yongxu Jan 3, 2017

yongxu commented Jan 8, 2017

loganfsmyth Jan 14, 2017

loganfsmyth Jan 14, 2017

yongxu Jan 17, 2017

yongxu Jan 17, 2017

babel-bot commented Jan 31, 2017

hzoo commented Feb 13, 2017

xtuc commented Feb 13, 2017

yongxu commented Feb 13, 2017

loganfsmyth commented Feb 14, 2017

yongxu commented Feb 14, 2017

loganfsmyth commented Feb 20, 2017

yongxu commented Feb 20, 2017

loganfsmyth commented Feb 20, 2017

olsonpm commented Mar 3, 2017 •

edited

yongxu commented Mar 7, 2017

Rewrite Hub as interface #5047 #5050

Rewrite Hub as interface #5047 #5050

Conversation

yongxu commented Dec 28, 2016 • edited by xtuc

codecov-io commented Dec 28, 2016 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xtuc Dec 30, 2016 • edited

Choose a reason for hiding this comment

yongxu commented Dec 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yongxu commented Jan 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

babel-bot commented Jan 31, 2017

hzoo commented Feb 13, 2017

xtuc commented Feb 13, 2017

yongxu commented Feb 13, 2017

loganfsmyth commented Feb 14, 2017

yongxu commented Feb 14, 2017

loganfsmyth commented Feb 20, 2017

yongxu commented Feb 20, 2017

loganfsmyth commented Feb 20, 2017

olsonpm commented Mar 3, 2017 • edited

yongxu commented Mar 7, 2017

yongxu commented Dec 28, 2016 •

edited by xtuc

codecov-io commented Dec 28, 2016 •

edited

xtuc Dec 30, 2016 •

edited

olsonpm commented Mar 3, 2017 •

edited