Skip to content

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

Merged
nicolo-ribaudo merged 2 commits intobabel:masterfrom
nicolo-ribaudo:placeholders/2-types
Mar 7, 2019
Merged

Add placeholders support to @babel/types and @babel/generator#9542
nicolo-ribaudo merged 2 commits intobabel:masterfrom
nicolo-ribaudo:placeholders/2-types

Conversation

@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Feb 19, 2019 •
edited
Loading

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 •
edited
Loading

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

@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/10177/

@nicolo-ribaudo nicolo-ribaudo added the PR: Needs Review A pull request awaiting more approvals label Feb 21, 2019
danez
danez reviewed Feb 26, 2019
View reviewed changes
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

nicolo-ribaudo commented Feb 26, 2019

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

existentialism commented Mar 5, 2019

%%:+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
@mAAdhaTTah
Merge branch 'master' into implement-f#-pipeline-in-parser
7458063 
* 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.

Reviewers

@existentialism existentialism existentialism approved these changes

+1 more reviewer

@danez danez danez approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

v7.4.0

Development

Successfully merging this pull request may close these issues.

4 participants

@nicolo-ribaudo @babel-bot @existentialism @danez