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 parser support for placeholders #9364

Merged
merged 19 commits into from Mar 4, 2019

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jan 18, 2019

Q                       A
Fixed Issues? #9112
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature? 👍
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

I added support for %%foo%% style placeholders to @babel/parser throught the placeholders plugin.
Part of this PR, which is a small refactor of import/export parsing, is split up at #9326 since it can be reviewd by its own.

For now I only allowed the Placeholder :: %% Identifier %% syntax, it could be expanded later. @babel/types, @babel/generator and @babel/template support will come in separate PRs.

cc @tolmasky

@nicolo-ribaudo nicolo-ribaudo added PR: New Feature 🚀 A type of pull request used for our changelog categories pkg: parser labels Jan 18, 2019
export type Placeholder<N: PlaceholderTypes> = NodeBase & {
type: "Placeholder",
id: Identifier,
expectedNode: N,
Copy link
Member Author

Choose a reason for hiding this comment

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

What do you prefer?

  • expectedNode
  • expectedType
  • expected
  • original

@babel-bot
Copy link
Collaborator

babel-bot commented Jan 18, 2019

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

@tolmasky
Copy link
Contributor

I just wanted to ask once again why we aren't using {% %} (I'm totally OK with %% if {% can't be done!), I just know that {%%} gets used for a lot of templating systems and it would be cool to allow it to fit naturally into those. In particular I am currently considering gitbook.io, where it would be nice to be able to use the same templating syntax in both the markdown itself as well as the codeblocks. Again though, I'm open to a different thing just wanted to raise this.

@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Jan 18, 2019

It is mainly because of https://github.com/keithamus/proposal-object-freeze-seal-syntax. Currently it uses {| ... |} and {# ... #}, but there are two different issues asking to replace {# with something else.
If it ends up using {%, we would have to change our placeholder syntax.

If no one thinkgs that that proposal could use {%, or has other suggestions, I'm open to changing %%.

If the "open-close" tokens pair is better, maybe <% ... %>?

packages/babel-parser/src/parser/util.js Outdated Show resolved Hide resolved
@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Jan 22, 2019

@keithamus Do you think that the {% %} syntax could conflict with your proposal?

@nicolo-ribaudo
Copy link
Member Author

Rebased and fixed #9364 (comment)

@keithamus
Copy link
Member

@nicolo-ribaudo {% %} will not conflict with my proposal as it stands, but I think your point around the potential for conflicting changes is true.

NOTE: Since %%FOO%% is parsed as an Expression, the following arrow
functions are not valid:

    %%FOO%% => {};        async %%FOO%% => {};

The heads must be parenthesized:

    (%%FOO%%) => {};      async (%%FOO%%) => {};

Also, placeholders must be parenthesized when used inside decorators:

    Invalid:              Valid:
    @%%DECO%%             @(%%DECO%%)
    class X {}            class X {}
@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Feb 25, 2019

Rebased on top of the scope tracking PR. The only changes are in the last two commits

@buildsize
Copy link

buildsize bot commented Feb 25, 2019

File name Previous Size New Size Change
babel-preset-env.js 2.02 MB 2.02 MB 7.39 KB (0%)
babel-preset-env.min.js 1.08 MB 1.08 MB 4.48 KB (0%)
babel.js 2.74 MB 2.75 MB 7.39 KB (0%)
babel.min.js 1.53 MB 1.54 MB 4.48 KB (0%)

Co-Authored-By: nicolo-ribaudo <nicolo.ribaudo@gmail.com>
@danez
Copy link
Member

danez commented Feb 25, 2019

File name Previous Size New Size Change
babel-preset-env.js 2.02 MB 2.02 MB 7.39 KB (0%)
babel-preset-env.min.js 1.08 MB 1.08 MB 4.48 KB (0%)
babel.js 2.74 MB 2.75 MB 7.39 KB (0%)
babel.min.js 1.53 MB 1.54 MB 4.48 KB (0%)

Off Topic: I changed the threshold to 30KB as the threshold is the total of changes for all assets together, so I calculated as 10KB per bundle + 5KB per minified bundle.

@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 Feb 26, 2019
@nicolo-ribaudo nicolo-ribaudo merged commit d832c0f into babel:master Mar 4, 2019
@nicolo-ribaudo nicolo-ribaudo deleted the placeholders/1-parser branch March 4, 2019 23:46
nicolo-ribaudo added a commit that referenced this pull request Mar 18, 2019
This is the last step to make #9364 usable in Babel. I'm sorry for opening this PR so late, but I hope to get it in v7.4.

In this PR I added a new option to `@babel/template`, `syntacticPlaceholders: ?boolean`, which toggles between `%%foo%%` placeholders (when `true`) and `FOO` placeholders. If it isn't specified, Babel tries to be "smart" to avoid breaking backward compat: if `%%foo%%` is used `syntacticPlaceholders` defaults to `true`, otherwise to `false`.

0e58e25 commit shows how some templates we used could be simplified by using this new placeholders syntax (we can't actually do it yet because we are importing `template` from `@babel/core` which could be an older version).

NOTE: Since I wanted to keep this PR as small as possible to make it easier to review, I didn't migrate `template.ast` to internally use the new syntax. It is an implementation detail, so it will be possible to change it in a patch release.
@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: parser 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

6 participants