Partial Application Syntax: Stage 1 by byara · Pull Request #9343 · babel/babel

byara · 2019-01-15T14:38:32Z

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

I modified parseExprListItem and added a new condition that checks for tt.comma and another boolean called allowPlaceholder. Then I check for the plugin and add the node Partial
I added a boolean argument allowPlaceholder in parseCallExpressionArguments and parseExprListItem to able to check if using ? is allowed or not, so I can throw and error.
I added 9 tests.

babel-bot · 2019-01-15T14:42:33Z

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

nicolo-ribaudo

Thank you for working on this proposal! I think that it could help having better discussions around the pipeline proposal.

cc @rbuckton (proposal champion) could you take a quick look at the tests?

nicolo-ribaudo · 2019-01-15T21:25:22Z

packages/babel-parser/src/parser/expression.js


+      case tt.question:
+        node = this.startNode();
+        this.raise(node.start, "Partial Application syntax is not allowed");


What are these lines needed for?

I wanted to be able to throw a message whenever the user is using an invalid syntax.
I added a boolean allowPlaceholder to parseExprListItem and parseCallExpressionArguments and thought, I could set it to false whenever we do not want to allow the usage of ?.
Maybe I could throw the error message with another conditional here:

} else if (this.match(tt.question)) { if (!allowPlaceholder){ const node = this.startNode(); this.raise(node.start, "Partial Application syntax is not allowed"); } this.expectPlugin("partialApplication"); const node = this.startNode(); this.next(); elt = this.finishNode(node, "Partial"); }

Also another solution could be to let Babel throw its own errors.

The default error would be unexpected ?. it's ok either way.

I like the allowPlaceholder idea from @byara , it can evolve into something more specific and make the users aware of when a partial application is allowed and when it's not, taking examples from https://github.com/tc39/proposal-partial-application#syntax we can tell the user we can point the user on the right direction instead of having him figure out why he's seeing an unexpected ? error.

nicolo-ribaudo · 2019-01-15T21:26:42Z

packages/babel-parser/ast/spec.md

 ## Super

+```js
+interface PartialExpression <: Node {


Maybe we could call it ArgumentPlaceholder?

Sure, I was just trying to follow the Grammar section of the proposal: Grammar. But I also think, ArgumentPlaceholder shows the purpose better.

nicolo-ribaudo · 2019-01-15T21:31:21Z

packages/babel-generator/src/generators/expressions.js

  }
 }

+export function Partial() {


This name doesn't match with the name defined in the parser. Also, could you add a generator test?

Sure, I'll add tests for the generator.
Do you mean in: packages/babel-parser/src/types.js
I see that I made a mistake there. I'll fix it 👍
I named it PartialExpression, thinking we can take it as an expression but remembered that the proposal does not consider this to be an expression.

Additionally, I suppose, I can move this function to src/generators/types.js? Because we are not considering Partial (or if we call it ArgumentPlaceholder) to be an expression.

Yeah good idea (I still prefer ArgumentPlaceholder)

byara · 2019-01-18T10:34:53Z

I renamed the node to ArgumentPlaceholder and added tests for the babel-generator.
I'm not sure about the type of the node though. In babel-types/src/validators/generated/index.js and babel-types/src/definitions/experimental.js I have put the node under UnaryLike but I'm not sure about it.
After I made the change in babel-types/src/validators/generated/index.js, adding ArgumentPlaceholder under UnaryLike, make build, creates a few functions automatically, not sure if that is normal.
for now, I removed the Error message, but if we decide it is a good idea, I'll put it back.
(I'm also working on my own implementation of the plugin for this proposal)

nicolo-ribaudo · 2019-01-18T10:52:56Z

I renamed the node to ArgumentPlaceholder and added tests for the babel-generator.

👍

I'm not sure about the type of the node though. In babel-types/src/validators/generated/index.js and babel-types/src/definitions/experimental.js I have put the node under UnaryLike but I'm not sure about it.

? is not an unary operator, I don't think that there should be any alias.

After I made the change in babel-types/src/validators/generated/index.js, adding ArgumentPlaceholder under UnaryLike, make build, creates a few functions automatically, not sure if that is normal.

All the files inside */generated/* are automatically created by the build process. You should only edit the files inside definitions.

byara · 2019-01-18T11:11:27Z

? is not an unary operator, I don't think that there should be any alias.

removed the alias

All the files inside /generated/ are automatically created by the build process. You should only edit the files inside definitions.

removed my changes from the generated folder.

I'll try to add more test cases for the parser and generator.

nicolo-ribaudo

removed my changes from the generated folder.

Sorry if I wasn't clear; I meant that it is correct to have those auto-generated files.

nicolo-ribaudo · 2019-01-20T23:31:20Z

packages/babel-types/src/definitions/experimental.js

+  visitor: ["argument"],
+  fields: {
+    argument: {
+      validate: assertNodeType("ArgumentPlaceholder"),


This would mean that the AST is something like this:

{ "type": "ArgumentPlaceholder", "argument": { "type": "ArgumentPlaceholder", // ... } }

It should only be this:

defineType("ArgumentPlaceholder", {});

nicolo-ribaudo · 2019-01-20T23:32:49Z

packages/babel-parser/src/parser/expression.js

      if (refTrailingCommaPos && this.match(tt.comma)) {
        refTrailingCommaPos.start = this.state.start;
      }
+    } else if (allowPlaceholder && this.match(tt.question)) {


Wdyt about this logic, to have better errors?

} else if (this.match(tt.question)) { this.expectPlugin("partialApplication"); if (allowPlaceholder) { this.raise(this.state.start, "Unexpected argument placeholder"); } // ...

I like this! It has a clear message and does not confuse the user. The user will understand that they should not use ArgumentPlaceholder there. I'll apply it in my next push.

P.S. I should apply the negated one right?

if (!allowPlaceholder) { this.raise(this.state.start, "Unexpected argument placeholder"); }

nicolo-ribaudo · 2019-01-20T23:33:13Z

packages/babel-parser/ast/spec.md

    - [ForStatement](#forstatement)
    - [ForInStatement](#forinstatement)
-    - [ForOfStatement](#forofstatement)
+  - [ForOfStatement](#forofstatement)


Are these indent changes auto-generated? 🤔

Yeah, I didn't change anything about the list, I just added my change in spec and the list was generated for me 🤔

I don't think this file is autogenerated, maybe it was one of your ide extensions, or formatter?
Can you revert this line and the one above in Changes?

You are right. Apparently, Markdown All in One was doing auto formatting.

nicolo-ribaudo · 2019-01-22T13:48:22Z

packages/babel-types/src/definitions/experimental.js

  classMethodOrDeclareMethodCommon,
 } from "./es2015";

+defineType("ArgumentPlaceholder", {});


We should also update the CallExpression definition to accept placeholder arguments

@nicolo-ribaudo You mean in /packages/babel-types/src/definitions/core.js I have to add another assert node type like this:

defineType("CallExpression", { visitor: ["callee", "arguments", "typeParameters", "typeArguments"], builder: ["callee", "arguments"], aliases: ["Expression"], fields: { callee: { validate: assertNodeType("Expression"), }, arguments: { validate: chain( assertValueType("array"), assertEach( assertNodeType( "Expression", "SpreadElement", "JSXNamespacedName", "ArgumentPlaceholder", ), ), ), }, ...

nicolo-ribaudo · 2019-01-23T21:38:25Z

packages/babel-plugin-syntax-partial-application/package.json

+{
+  "name": "@babel/plugin-syntax-partial-application",
+  "version": "7.2.0",
+  "description": "Allow parsing of optional properties",


This description needs to be updated

byara · 2019-01-24T17:56:25Z

~~@nicolo-ribaudo I was looking through the changes and I noticed that I have missed ~~a few~~ one ~~things~~ thing:~~

~~forbidding the usage with SpreadElement~~
~~allowing the usage in TemplateLiteral~~
~~What do you think? Should I address them in this PR? or another one?~~

rbuckton · 2019-01-24T18:36:19Z

Partial application in tagged templates was pretty much rejected in a previous TC39 meeting, so I plan to remove that from the proposal for the time being.

nicolo-ribaudo · 2019-01-24T20:28:24Z

forbidding the usage with SpreadElement

Do you mean that foo(?, ...arr) isn't valid? Or are you talking about foo(...)?

byara · 2019-01-24T21:08:16Z

forbidding the usage with SpreadElement

Do you mean that foo(?, ...arr) isn't valid? Or are you talking about foo(...)?

Right now, we can parse foo(?, ...arr). But as I was reading this part of the proposal, I had the impression that maybe we shouldn't parse it?

nicolo-ribaudo · 2019-01-24T21:11:56Z

I think that that part is only disallowing the old proposed foo(...) syntax (rest placeholder), not foo(?, ...a).

byara · 2019-01-24T21:18:31Z

I think that that part is only disallowing the old proposed foo(...) syntax (rest placeholder), not foo(?, ...a).

I knew I got it wrong! Then I guess everything is alright!
I'm also ready to make a PR for the plugin (It's like 90% done).

nicolo-ribaudo · 2019-01-24T21:33:44Z

Lets just wait for anather review so that this PR can be merged. Can you rebase it in the meantime?

byara · 2019-01-24T21:36:51Z

Lets just wait for anather review so that this PR can be merged. Can you rebase it in the meantime?

Sure! I'll do that 👍

danez

Looks good to me, I only added some comments about some nits in the markdown file.
Thank you

danez · 2019-02-12T21:27:39Z

packages/babel-parser/ast/spec.md

 }
 ```

+## ArgumentPlaceholder


on more level please ###

danez · 2019-02-12T21:30:09Z

packages/babel-parser/ast/spec.md

    - [LogicalExpression](#logicalexpression)
      - [LogicalOperator](#logicaloperator)
    - [SpreadElement](#spreadelement)
+  - [ArgumentPlaceholder](#argumentplaceholder)


should also be intended one more level.

danez · 2019-02-12T21:30:14Z

packages/babel-parser/ast/spec.md

    - [ForStatement](#forstatement)
    - [ForInStatement](#forinstatement)
-    - [ForOfStatement](#forofstatement)
+  - [ForOfStatement](#forofstatement)


I don't think this file is autogenerated, maybe it was one of your ide extensions, or formatter?
Can you revert this line and the one above in Changes?

danez · 2019-02-12T21:30:28Z

packages/babel-parser/ast/spec.md


 Any expression node. Since the left-hand side of an assignment may be any expression in general, an expression can also be a pattern.

+


not necessary

nicolo-ribaudo · 2019-02-19T21:04:39Z

@byara Thank you! We will merge this PR when we decide to release v7.4.0. In the meantime, could you squash it and rebase the other PR on top of this one?

existentialism

Nice work 👍

nicolo-ribaudo · 2019-03-04T23:28:54Z

I'm merging this locally since it has some conflicts

* add partial application syntax and some tests * remove unnecessary error message and hasPartial function from parseNewArguments * add types for PartialExpression * Update the tests * rename PartialExpression to Partial * move Partial from expressions to types and rename to ArgumentPlaceholder * add tests for ArgumentPlaceholder in babel-generator * rename Partial to ArgumentPlaceholder * update the tests * remove alias from the type and undo changes in generated folder * adds a nice error message * better definition for the type * auto-generated files * update the conditional for allowPlaceholder message and tests * update CallExpression definition to accept ArgumentPlaceholder * change description * clean up * indent ArgumentPlaceholder entry and revert unwanted changes

nicolo-ribaudo reviewed Jan 15, 2019

View reviewed changes

nicolo-ribaudo added PR: New Feature 🚀 A type of pull request used for our changelog categories pkg: parser Spec: Partial Application labels Jan 15, 2019

nicolo-ribaudo reviewed Jan 15, 2019

View reviewed changes

byara force-pushed the master branch from 65cdb98 to 1acd6a4 Compare January 18, 2019 11:08

nicolo-ribaudo reviewed Jan 20, 2019

View reviewed changes

nicolo-ribaudo reviewed Jan 22, 2019

View reviewed changes

nicolo-ribaudo reviewed Jan 23, 2019

View reviewed changes

nicolo-ribaudo approved these changes Jan 23, 2019

View reviewed changes

byara force-pushed the master branch from 0a01daa to 53879cd Compare January 25, 2019 16:25

nicolo-ribaudo added this to the v7.4.0 milestone Feb 11, 2019

danez suggested changes Feb 12, 2019

View reviewed changes

byara mentioned this pull request Feb 18, 2019

Partial application plugin #9474

Merged

danez approved these changes Feb 19, 2019

View reviewed changes

nicolo-ribaudo added the PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release label Feb 19, 2019

byara force-pushed the master branch from 09a47fe to aa5cb0f Compare February 20, 2019 09:49

existentialism approved these changes Feb 20, 2019

View reviewed changes

existentialism mentioned this pull request Feb 20, 2019

Add documentation for Partial Application Syntax: Stage 1 babel/website#1974

Closed

4 tasks

nicolo-ribaudo closed this Mar 4, 2019

nicolo-ribaudo force-pushed the master branch from aa5cb0f to 37a427f Compare March 4, 2019 23:27

nicolo-ribaudo changed the title ~~Partial Application Syntax: Stage 1~~ [MERGED] Partial Application Syntax: Stage 1 Mar 4, 2019

nicolo-ribaudo changed the title ~~[MERGED] Partial Application Syntax: Stage 1~~ Partial Application Syntax: Stage 1 Mar 16, 2019

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 bot locked as resolved and limited conversation to collaborators Oct 4, 2019


		Any expression node. Since the left-hand side of an assignment may be any expression in general, an expression can also be a pattern.

Uh oh!

Conversation

byara commented Jan 15, 2019

Uh oh!

babel-bot commented Jan 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

byara Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

byara Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

byara Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

byara commented Jan 18, 2019

Uh oh!

nicolo-ribaudo commented Jan 18, 2019

Uh oh!

byara commented Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolo-ribaudo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

byara Jan 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danez Feb 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

byara Jan 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

byara commented Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rbuckton commented Jan 24, 2019

Uh oh!

nicolo-ribaudo commented Jan 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

babel-bot commented Jan 15, 2019 •

edited

Loading

byara Jan 16, 2019 •

edited

Loading

byara Jan 16, 2019 •

edited

Loading

byara Jan 16, 2019 •

edited

Loading

byara commented Jan 18, 2019 •

edited

Loading

byara Jan 21, 2019 •

edited

Loading

danez Feb 12, 2019 •

edited

Loading

byara Jan 23, 2019 •

edited

Loading

byara commented Jan 24, 2019 •

edited

Loading

nicolo-ribaudo commented Jan 24, 2019 •

edited

Loading

byara commented Jan 24, 2019 •

edited

Loading

danez Feb 12, 2019 •

edited

Loading

nicolo-ribaudo commented Mar 4, 2019 •

edited

Loading