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

Parenthesized expressions #8025

Merged
merged 2 commits into from Feb 23, 2019

Conversation

@arv
Copy link
Contributor

commented May 24, 2018

Q                       A
Fixed Issues? Fixes #7661, closes #7015
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature? New feature
Tests Added + Pass? Yes
Documentation PR
Any Dependency Changes? No
License MIT

This adds a parser option called createParenthesizedExpressions, which when enabled creates ParenthesizedExpression nodes instead of setting extra.parenthesized.

@arv arv force-pushed the arv:parenthesized-expressions branch 4 times, most recently from dfe4825 to eff42c6 May 24, 2018
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

commented May 24, 2018

  1. I think that this shouldn't be a Babylon plugin (since plugins are meant to enable new syntax), but it should just be an option (which you already implemented).

  2. Can you add a test about (foo) => {} works with this new option enabled?

@arv arv force-pushed the arv:parenthesized-expressions branch from eff42c6 to 8e9629a May 25, 2018
@arv

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2018

All done!

  1. Removed the parser plugin
  2. Added more tests and fixed/improved issues related to lval.
@babel-bot

This comment has been minimized.

Copy link
Collaborator

commented May 25, 2018

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

@babel-bot

This comment has been minimized.

Copy link
Collaborator

commented May 25, 2018

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

@@ -0,0 +1,3 @@
{
"parserOpts": {"createParenthesizedExpressions": true}

This comment has been minimized.

Copy link
@arv

arv May 25, 2018

Author Contributor

This works because I updated the runner in packages/babel-generator/test/index.js to spread the parserOpts into the parser options.

@arv arv force-pushed the arv:parenthesized-expressions branch from 8e9629a to e1b1b23 May 30, 2018
@arv

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2018

Rebased on top of 2a8ebbe

@arv arv force-pushed the arv:parenthesized-expressions branch from e1b1b23 to f77818a Nov 6, 2018
@arv

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

I'm still interested in getting this feature in. We are using Babel more and more in our tools and using a fork is getting old. Any chance of this getting another review?

@hzoo @nicolo-ribaudo

@arv

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2019

This is still blocking us. Any suggestions on how to proceed?

CC @rafaelweinstein

@nicolo-ribaudo nicolo-ribaudo self-requested a review Feb 19, 2019
@jridgewell

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

AMP Project also needed this. We solved it by running a custom plugin as the first step in our babel config:

// https://github.com/ampproject/amphtml/blob/2dc43d8df8a899f95348c2149aceddc9ca96b5a0/build-system/babel-plugins/babel-plugin-transform-parenthesize-expression/index.js
module.exports = function(babel) {
  const {types: t} = babel;
  return {
    visitor: {
      Expression: {
        exit(path) {
          const {node} = path;
          const {parenthesized} = node.extra || {};
          if (!parenthesized) {
            return;
          }

          path.replaceWith(t.parenthesizedExpression(node));
          path.skip();
        },
      },
    },
  };
};

A babel-parser config option would be welcome.

Copy link
Member

left a comment

Apart from some minor comments, this PR looks pretty good.

return val;
const parenExpression = this.startNodeAt(startPos, startLoc);
parenExpression.expression = val;
this.finishNodeAt(

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Feb 19, 2019

Member

Isn't using this.finishNode enough? The end position should be the correct one at this point I think.

@@ -0,0 +1 @@
({x}) = {x: 1};

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Feb 19, 2019

Member

Could you also test ([ a ]) = []?

This comment has been minimized.

Copy link
@arv

arv Feb 20, 2019

Author Contributor

Done

@danez danez self-requested a review Feb 19, 2019
Copy link
Contributor Author

left a comment

  • resolved the merge conflicts
  • Use finishNode instead of finishNodeAt
  • add test for ([a]) = []
  • remove dead code (parseParenExpression)
@@ -0,0 +1 @@
({x}) = {x: 1};

This comment has been minimized.

Copy link
@arv

arv Feb 20, 2019

Author Contributor

Done

@@ -19,6 +19,7 @@ export type Options = {
strictMode: ?boolean,
ranges: boolean,
tokens: boolean,
createParenthesizedExpressions: boolean,

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Feb 20, 2019

Member

This also needs to be added to babel-parser/typings/index.d.ts

This comment has been minimized.

Copy link
@arv

arv Feb 20, 2019

Author Contributor
@danez
danez approved these changes Feb 20, 2019
@arv

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2019

Thanks @danez

@nicolo-ribaudo nicolo-ribaudo added this to the v7.4.0 milestone Feb 20, 2019
Copy link
Member

left a comment

👍

arv added 2 commits Feb 20, 2019
When set to `true` we create `ParenthesizedExpression` nodes instead of
setting `extra.parenthesized`.
@jridgewell jridgewell force-pushed the arv:parenthesized-expressions branch from 3f2e477 to 88ed72b Feb 23, 2019
@jridgewell jridgewell merged commit dd8b700 into babel:master Feb 23, 2019
4 checks passed
4 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 87% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

commented Feb 23, 2019

We were waiting to decide to release v7.4 😛

@jridgewell

This comment has been minimized.

Copy link
Member

commented Feb 23, 2019

Whoops, didn't see the milestone.

jridgewell added a commit that referenced this pull request Feb 23, 2019
This reverts commit dd8b700.
nicolo-ribaudo added a commit that referenced this pull request Feb 25, 2019
This reverts commit dd8b700.
nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Feb 25, 2019
* Add parser createParenthesizedExpressions option  …

When set to `true` we create `ParenthesizedExpression` nodes instead of
setting `extra.parenthesized`.

* Also update babel-parser.d.ts
nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Mar 3, 2019
* Add parser createParenthesizedExpressions option  …

When set to `true` we create `ParenthesizedExpression` nodes instead of
setting `extra.parenthesized`.

* Also update babel-parser.d.ts
nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Mar 5, 2019
* Add parser createParenthesizedExpressions option  …

When set to `true` we create `ParenthesizedExpression` nodes instead of
setting `extra.parenthesized`.

* Also update babel-parser.d.ts
nicolo-ribaudo added a commit that referenced this pull request Mar 6, 2019
* Add parser createParenthesizedExpressions option  …

When set to `true` we create `ParenthesizedExpression` nodes instead of
setting `extra.parenthesized`.

* Also update babel-parser.d.ts
@lock lock bot added the outdated 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.
You can’t perform that action at this time.