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

Create InterpreterDirective AST node type and use to replace babel/core File's 'shebang' handling #7928

Merged
merged 4 commits into from May 22, 2018

Conversation

Projects
None yet
4 participants
@loganfsmyth
Member

loganfsmyth commented May 14, 2018

Q                       A
Fixed Issues? Fixes #6586, Fixes #7905
Patch: Bug Fix? Y
Major: Breaking Change? N
Minor: New Feature? Y
Tests Added + Pass? Yes
Documentation PR
Any Dependency Changes?
License MIT

Both of these bugs were essentially because of the way we removed the shebang up front, meaning it was totally separated from the normal babel-generator codegen process.

This is another divergence from ESTree, but we've also got https://github.com/bmeck/proposal-hashbang as an active proposal anyway, so it seems like adding an AST node for this isn't too unreasonable.

I was considering adding a Babylon plugin and auto-enabling it in babel-core, but then I realized Babylon was already parsing shebangs and sticking it in a comment, which I think is even weirder. So this essentially means that any third-parties using Babylon directly and not through Babel will see changed behavior, but no-one using Babel.

@babel-bot

This comment has been minimized.

Show comment
Hide comment
@babel-bot

babel-bot May 14, 2018

Collaborator

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

Collaborator

babel-bot commented May 14, 2018

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

@loganfsmyth loganfsmyth changed the title from Create Shebang AST node type and use to replace babel/core shebang handling to Create InterpreterDirective AST node type and use to replace babel/core File's 'shebang' handling May 15, 2018

@xtuc

This comment has been minimized.

Show comment
Hide comment
@xtuc

xtuc May 19, 2018

Member

I think the interpreter key should be optional in the AST. It doesn't really make sense for scripts/modules meant for the web?

I like the way you introduced that because we could have an option/plugin that automatically adds a shebang to the generated file.

Member

xtuc commented May 19, 2018

I think the interpreter key should be optional in the AST. It doesn't really make sense for scripts/modules meant for the web?

I like the way you introduced that because we could have an option/plugin that automatically adds a shebang to the generated file.

@loganfsmyth loganfsmyth merged commit d164f82 into babel:master May 22, 2018

4 checks passed

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

@loganfsmyth loganfsmyth deleted the loganfsmyth:shebang-node branch May 22, 2018

nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request May 22, 2018

nicolo-ribaudo added a commit that referenced this pull request May 22, 2018

existentialism added a commit to prettier/prettier that referenced this pull request May 25, 2018

Switch to @babel/parser (#4544)
We landed a change that added a new `InterpreterDirective` AST node type for hashbangs, and no longer add it as a comment/leadingComment.

Ref: babel/babel#7928

I mimicked what we do in `@babel/generator` here, since I found it better than trying to add comments to the ast in `parser-include-shebang.js`). Definitely open to a better/cleaner option though!

Note: I'll follow this up with enabling tests for #4543 and #4540 once they land too.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment