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

Export @babel/parser#tokTypes in @babel/core #8986

Merged
merged 2 commits into from Dec 1, 2018

Conversation

Projects
None yet
8 participants
@kaicataldo
Copy link
Member

kaicataldo commented Nov 7, 2018

Q                       A
Fixed Issues? Refs https://github.com/babel/babel-eslint/pull/711/files#r231363345
Patch: Bug Fix? No
Major: Breaking Change? No
Minor: New Feature? Yes
Tests Added + Pass? Yes
Documentation PR Link N/A
Any Dependency Changes? No
License MIT

This exposes @babel/parser in @babel/core, as proposed here. Figured it would be best to just make a PR that we could discuss on :) I wasn't sure if testing deeper than this would be worth it or not, given it would have to change any time @babel/parser's API changes, but happy to update as the team sees fit. Thanks!

@kaicataldo kaicataldo referenced this pull request Nov 7, 2018

Merged

Use @babel/core#parse #711

@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Nov 7, 2018

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/9471/

@ljharb

ljharb approved these changes Nov 7, 2018

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Nov 7, 2018

Doesn't babelCore.parse work for your usecase?

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Nov 7, 2018

Oh nvm, I saw the babel-eslint pr

@kaicataldo

This comment has been minimized.

Copy link
Member Author

kaicataldo commented Nov 7, 2018

@nicolo-ribaudo For the PR I'm working on, all I actually need is babelParser.tokTypes. The idea was first brought up by @loganfsmyth here, but I know a lot has changed since then.

I do think it's weird/confusing to have babelCore.parse and babelCore.parser.parse though 😬

@xtuc

xtuc approved these changes Nov 8, 2018

Copy link
Member

xtuc left a comment

Missing documentation on the website, but LGTM, thanks

@danez

danez approved these changes Nov 8, 2018

Copy link
Member

danez left a comment

We should document it under advanced documentation or not document it in my opinion, because babelCore.parse and babelCore.parser.parse do different things in terms of config, and I could see people getting confused about this.

@loganfsmyth

This comment has been minimized.

Copy link
Member

loganfsmyth commented Nov 9, 2018

Revisiting this after my one-off comment that prompted this, I'm concerned because everything we add to the root by default is exposed in plugins/presets on the api object, and this doesn't seem like something we'd actually want there, especially since we already expose template as the suggested way to create ASTs from code snippets, and parse as a way to parse using the user's config.

I'm not necessarily against this being exported from @babel/core, but I'd want to avoid exposing on api. I do wonder if we'd be better off exporting tokTypes directly instead. I didn't realize until now what tokTypes even was. It's really unfortunate that the tokens array relies on object identity for checking token types :(

@babel babel deleted a comment from babel-bot Nov 9, 2018

@xtuc

This comment has been minimized.

Copy link
Member

xtuc commented Nov 9, 2018

I agree with Logan, and I'm not a big fan of this implicitly exposed API. Would it be possible to change the API to be a fixed object? and then you can expose the parse function.

@kaicataldo

This comment has been minimized.

Copy link
Member Author

kaicataldo commented Nov 10, 2018

@xtuc For this PR I actually need access to tokTypes in @babel/parser, but I mainly made this PR because @loganfsmyth had requested it. I'm fine with just closing this and doing what we're doing now. Or would you prefer for me to leave this open as a place of discussion about how we should move forward?

@ljharb

This comment has been minimized.

Copy link

ljharb commented Nov 10, 2018

I would greatly appreciate being able to use the parser in a transform without needing an explicit dependency on it; it would be great to find a way to do that.

@xtuc

This comment has been minimized.

Copy link
Member

xtuc commented Nov 10, 2018

As @ljharb mentioned, exposing the parser in a transform and correctly configured does solve a problem.

I would be in favor of leaving this open for discussion, because we should figure a way to do it I guess.

@kaicataldo

This comment has been minimized.

Copy link
Member Author

kaicataldo commented Nov 12, 2018

Actually, seems like an issue would be a better way to track this discussion. At this point, I'm not going to pursue this change since it's not necessary for the babel-eslint PR.

@loganfsmyth

This comment has been minimized.

Copy link
Member

loganfsmyth commented Nov 13, 2018

@kaicataldo Can we just re-export tokTypes on the object for now, and add tokTypes: undefined to

so the value will be overwritten with a blank value? Then we can always go in later to trim down the plugin API.

@kaicataldo kaicataldo force-pushed the kaicataldo:expose-parser-in-core branch from 60667e2 to 63a6a9b Nov 27, 2018

@kaicataldo kaicataldo force-pushed the kaicataldo:expose-parser-in-core branch from 63a6a9b to 08734cc Nov 27, 2018

@kaicataldo

This comment has been minimized.

Copy link
Member Author

kaicataldo commented Nov 27, 2018

@loganfsmyth Apologies for the delay - PR updated.

@kaicataldo kaicataldo changed the title Export @babel/parser in @babel/core Export @babel/parser#tokTypes in @babel/core Nov 27, 2018

@loganfsmyth loganfsmyth merged commit 806e133 into babel:master Dec 1, 2018

4 checks passed

babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 80.61% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kaicataldo kaicataldo deleted the kaicataldo:expose-parser-in-core branch Dec 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment