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

Add placeholders support to @babel/types and @babel/generator #9542

Merged
merged 2 commits into from Mar 7, 2019

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Feb 19, 2019

Q                       A
Minor: New Feature? Yes
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

Related to #9364, but those two PRs can be merged in any order.

The "core" of this PR is at https://github.com/babel/babel/pull/9542/files#diff-70f395c5d000299ece93fa4d13b656f9R18: does it make sense? That same logic is replicated for the is[Type] helpers.

@nicolo-ribaudo nicolo-ribaudo added PR: New Feature 🚀 A type of pull request used for our changelog categories pkg: generator pkg: types labels Feb 19, 2019
@nicolo-ribaudo nicolo-ribaudo added this to the v7.4.0 milestone Feb 19, 2019
@babel-bot
Copy link
Collaborator

babel-bot commented Feb 19, 2019

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

@babel-bot
Copy link
Collaborator

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

@nicolo-ribaudo nicolo-ribaudo added the PR: Needs Review A pull request awaiting more approvals label Feb 21, 2019
Copy link
Member

@danez danez left a comment

Choose a reason for hiding this comment

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

The change makes sense to me and not controversial at all.
I guess that most of the time nobody will see Placeholders in the AST anyway as it will be exclusively used by @babel/template, right?

That makes me wonder though if the changes to @babel/types are actually necessary? As long as
there is no error thrown I would think they are not really needed unless @babel/template needs them?

@nicolo-ribaudo
Copy link
Member Author

Yeah, 99% of the times they will only be used by @babel/template. But I think that we should add them to @babel/types for two reasons:

  1. Every other node is defined in @babel/types, even nodes that you will almost never use (e.g. t.isFile).
  2. Even if they will mostly be used inside @babel/template, there are users which want to use placeholders in custom transforms. For example, point 3 of Support @babel/template Placeholders at the Syntax Level in @babel/parser #9112 (comment)

@existentialism
Copy link
Member

%%:+1:%%

@nicolo-ribaudo nicolo-ribaudo added PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release and removed PR: Needs Review A pull request awaiting more approvals labels Mar 5, 2019
@nicolo-ribaudo nicolo-ribaudo merged commit 15dfce3 into babel:master Mar 7, 2019
@nicolo-ribaudo nicolo-ribaudo deleted the placeholders/2-types branch March 7, 2019 10:47
mAAdhaTTah added a commit to mAAdhaTTah/babel that referenced this pull request Mar 15, 2019
* master: (58 commits)
  Remove dependency on home-or-tmp package (babel#9678)
  [proposal-object-rest-spread] fix templateLiteral in extractNormalizedKeys (babel#9628)
  Partial application plugin (babel#9474)
  Private Static Class Methods (Stage 3) (babel#9446)
  gulp-uglify@3.0.2
  rollup@1.6.0
  eslint@5.15.1
  jest@24.5.0
  regexpu-core@4.5.4
  Remove input and length from state (babel#9646)
  Switch from rollup-stream to rollup and update deps (babel#9640)
  System modules - Hoist classes like other variables (babel#9639)
  fix: Don't transpile ES2018 symbol properties (babel#9650)
  Add WarningsToErrorsPlugin to webpack to avoid missing build problems on CI (babel#9647)
  Update regexpu-core dependency (babel#9642)
  Add placeholders support to @babel/types and @babel/generator (babel#9542)
  Generate plugins file
  Make babel-standalone an ESModule and enable flow (babel#9025)
  Reorganize token types and use a map for them (babel#9645)
  [TS] Allow context type annotation on getters/setters (babel#9641)
  ...
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: generator pkg: types PR: New Feature 🚀 A type of pull request used for our changelog categories PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants