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

Closed
yongxu opened this issue Dec 27, 2016 · 1 comment · Fixed by #5050 or #8469
Closed

Rewrite Hub as interface #5047

yongxu opened this issue Dec 27, 2016 · 1 comment · Fixed by #5050 or #8469
Labels
Has PR outdated A closed issue/PR that is archived due to age. Recommended to make a new issue Priority: High

Comments

@yongxu
Copy link
Contributor

yongxu commented Dec 27, 2016

The current babel-traverse implementation of Hub will cause bugs if this.hub.file does not exist, which is only injected by File class in babel-core.

This means babel-traverse package can not be used without babel-core unless we monkey-patch hub with a fake file object. The Hub class itself doesn't carry any useful information besides the file reference.

Defining hub as an interface instead of an object, will make babel-traverse a standalone package and resolve issues #4640 #4413

Solution

we can redefine Hub(or a better name maybe) as

// all the methods should be optional(or make hub optional?), so there will be no exception
declare interface Hub {
  mark(type: string, message: string): void;
  getCode(): string;
  getFileScope(): Scope; // or just getFile, maybe?
  onError(node: Object, msg: string, Error: typeof Error = SyntaxError): void;
}

This interface would be sufficient for the current version, and can be extended in the future.
It also can be checked by flowtype

Here are the whys, where Hub appears

// this function is only used in debugging
  mark(type: string, message: string) {
    this.hub.file.metadata.marked.push({
      type,
      message,
      loc: this.node.loc
    });
  }

// this can be replaced by getCode()
export function getSource() {
  let node = this.node;
  if (node.end) {
    return this.hub.file.code.slice(node.start, node.end);
  } else {
    return "";
  }
}

// JSX plugin
  let visitor = {
    JSXOpeningElement(path, state) {
//..........................................
          path.hub.file.scope.push({id: fileNameIdentifier, init: t.stringLiteral(fileName)});

// and called buildCodeFrameError in few places
throw this.hub.file.buildCodeFrameError

Please let me know if you have any suggestion, or better improvement! I will make a PR if I can get some positive faceback :)

more about hub

the current implementation of hub itself only consist 6 lines of code, and the options has never been used anywhere:

export default class Hub {
  constructor(file, options) {
    this.file = file;
    this.options = options;
  }
}

Here are all the places hub appears, when searching keyword hub in package/:
image

yongxu added a commit to yongxu/babel that referenced this issue Dec 27, 2016
@yongxu
Copy link
Contributor Author

yongxu commented Dec 27, 2016

yongxu@459b5dc
The potential PR for this issue. PR not submitted yet, cuz not tested yet.
Going to take a nap first :)

yongxu added a commit to yongxu/babel that referenced this issue Dec 28, 2016
yongxu added a commit to yongxu/babel that referenced this issue Dec 28, 2016
yongxu added a commit to yongxu/babel that referenced this issue Dec 28, 2016
yongxu added a commit to yongxu/babel that referenced this issue Dec 30, 2016
yongxu added a commit to yongxu/babel that referenced this issue Dec 30, 2016
yongxu added a commit to yongxu/babel that referenced this issue Dec 30, 2016
yongxu added a commit to yongxu/babel that referenced this issue Jan 3, 2017
yongxu added a commit to yongxu/babel that referenced this issue Jan 17, 2017
yongxu added a commit to yongxu/babel that referenced this issue Jan 17, 2017
loganfsmyth pushed a commit that referenced this issue Feb 13, 2017
* Rewrite Hub as interface #5047

* Update index.js
danez added a commit that referenced this issue Feb 14, 2017
* 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
existentialism pushed a commit that referenced this issue May 19, 2017
* Rewrite Hub as interface #5047

* Update index.js
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 5, 2018
@lock lock bot locked as resolved and limited conversation to collaborators May 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Has PR outdated A closed issue/PR that is archived due to age. Recommended to make a new issue Priority: High
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants