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

Rewrite Hub as interface #5047 #5050

Merged
merged 3 commits into from Feb 13, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 18 additions & 3 deletions packages/babel-core/src/transformation/file/index.js
Expand Up @@ -7,7 +7,8 @@ import convertSourceMap from "convert-source-map";
import OptionManager from "./options/option-manager";
import type Pipeline from "../pipeline";
import PluginPass from "../plugin-pass";
import { NodePath, Hub, Scope } from "babel-traverse";
import { NodePath, Scope } from "babel-traverse";
import type { HubInterface } from "babel-traverse";
import sourceMap from "source-map";
import generate from "babel-generator";
import codeFrame from "babel-code-frame";
Expand Down Expand Up @@ -99,7 +100,21 @@ export default class File extends Store {
this.code = "";
this.shebang = "";

this.hub = new Hub(this);
this.hub = {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice catch! fixed it :)

// keep it for the usage in babel-core, ex: path.hub.file.opts.filename
file: this,
mark: (type: string, message: string, loc: Object) => {
this.metadata.marked.push({
type,
message,
loc
});
},
addHelper: this.addHelper.bind(this),
getCode: () => this.code,
getScope: () => this.scope,
buildError: this.buildCodeFrameError.bind(this)
};
Copy link
Member

Choose a reason for hiding this comment

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

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

}

static helpers: Array<string>;
Expand All @@ -119,7 +134,7 @@ export default class File extends Store {
ast: Object;
scope: Scope;
metadata: BabelFileMetadata;
hub: Hub;
hub: HubInterface;
code: string;
shebang: string;

Expand Down
Expand Up @@ -50,7 +50,10 @@ 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();
if (scope) {
scope.push({id: fileNameIdentifier, init: t.stringLiteral(fileName)});
}
state.fileNameIdentifier = fileNameIdentifier;
}

Expand Down
14 changes: 11 additions & 3 deletions packages/babel-traverse/src/hub.js
@@ -1,6 +1,14 @@
import Scope from "./scope";
export interface HubInterface {
mark?: (type: string, message: string) => void;
addHelper?: (name: string) => Object;
getScope?: () => Scope;
getCode?: () => string;
buildError:(node: Object, msg: string, Error: Error) => Error;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

export default class Hub {
constructor(file, options) {
this.file = file;
this.options = options;
buildError(node, msg, BuildError = TypeError): Error {
return new BuildError(msg);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
}
16 changes: 9 additions & 7 deletions packages/babel-traverse/src/index.js
Expand Up @@ -7,10 +7,12 @@ import includes from "lodash/includes";
import * as t from "babel-types";
import * as cache from "./cache";

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

export { visitors, NodePath, Scope, Hub };
export type { HubInterface } from "./hub";

export default function traverse(
parent: Object | Array<Object>,
Expand All @@ -37,9 +39,9 @@ traverse.visitors = visitors;
traverse.verify = visitors.verify;
traverse.explode = visitors.explode;

traverse.NodePath = require("./path");
traverse.Scope = require("./scope");
traverse.Hub = require("./hub");
traverse.NodePath = NodePath;
traverse.Scope = Scope;
traverse.Hub = Hub;

traverse.cheap = function (node, enter) {
return t.traverseFast(node, enter);
Expand Down
10 changes: 4 additions & 6 deletions packages/babel-traverse/src/path/index.js
Expand Up @@ -116,19 +116,17 @@ export default class NodePath {
}

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

traverse(visitor: Object, state?: any) {
traverse(this.node, visitor, this.scope, state, this);
}

mark(type: string, message: string) {
this.hub.file.metadata.marked.push({
type,
message,
loc: this.node.loc
});
if (this.hub.mark) {
this.hub.mark(type, message, this.node.loc);
}
}

set(key: string, node: Object) {
Expand Down
8 changes: 5 additions & 3 deletions packages/babel-traverse/src/path/introspection.js
Expand Up @@ -235,10 +235,12 @@ export function referencesImport(moduleSource, importName) {
export function getSource() {
let node = this.node;
if (node.end) {
return this.hub.file.code.slice(node.start, node.end);
} else {
return "";
const code = this.hub.getCode();
if (code) {
return code.slice(node.start, node.end);
}
}
return "";
}

export function willIMaybeExecuteBefore(target) {
Expand Down
8 changes: 4 additions & 4 deletions packages/babel-traverse/src/scope/index.js
Expand Up @@ -350,7 +350,8 @@ export default class Scope {
if (!duplicate) duplicate = local.kind === "param" && (kind === "let" || kind === "const");

if (duplicate) {
throw this.hub.file.buildCodeFrameError(id, messages.get("scopeDuplicateDeclaration", name), TypeError);
const errorMsg = messages.get("scopeDuplicateDeclaration", name);
throw this.hub.buildError ? this.hub.buildError(id, errorMsg, TypeError) : new TypeError(errorMsg);
}
}

Expand Down Expand Up @@ -389,7 +390,6 @@ export default class Scope {
}

toArray(node: Object, i?: number) {
let file = this.hub.file;

if (t.isIdentifier(node)) {
let binding = this.getBinding(node.name);
Expand Down Expand Up @@ -423,9 +423,9 @@ export default class Scope {
} else if (i) {
args.push(t.numericLiteral(i));
helperName = "slicedToArray";
// TODO if (this.hub.file.isLoose("es6.forOf")) helperName += "-loose";
// TODO if (this.hub.isLoose("es6.forOf")) helperName += "-loose";
}
return t.callExpression(file.addHelper(helperName), args);
return t.callExpression(this.hub.addHelper(helperName), args);
}

hasLabel(name: string) {
Expand Down
13 changes: 13 additions & 0 deletions packages/babel-traverse/test/hub.js
@@ -0,0 +1,13 @@
const assert = require("assert");
const Hub = require("../lib").default.Hub;

describe("hub", function () {
it("default buildError should return TypeError", function () {
const hub = new Hub();
const msg = "test_msg";
assert.deepEqual(
hub.buildError(null, msg),
new TypeError(msg)
);
});
});