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 optionality to catch bindings #5956

Merged
merged 8 commits into from Jul 25, 2017

Conversation

@MarckK
Member

MarckK commented Jul 16, 2017

Q A
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature? yes
Deprecations?
Spec Compliancy?
Tests Added/Pass?
Fixed Tickets
License MIT
Doc PR
Dependency Changes

This implements the optional catch binding proposal (babel/proposals #7).

@jridgewell

We'll also need:

  • Change to babel-types to make param optional.
  • Change to babel-generator to output a param-less clause.
  • A transform that generates a unique Identifier if one is not present.
  • Tests for the above
visitor: {
CatchClause({ node }) {
if (!node.param) {
node.param.name = "_unused";

This comment has been minimized.

@bakkot

bakkot Jul 19, 2017

Contributor

This might conflict with something; also, if node.param is null or undefined, trying to set node.param.name will throw an error. You probably want generateUidIdentifier.

@@ -0,0 +1,3 @@
src

This comment has been minimized.

@existentialism

existentialism Jul 19, 2017

Member

This folder looks like it was inadvertently added?

## Detail
> Optional catch bindings enable the catch block to execute whether or not a parameter is passed to the catch statement (CatchClause).

This comment has been minimized.

@existentialism

existentialism Jul 19, 2017

Member

Nit: enable -> enables

if (node.param.name) {
return;
} else {
const uid = scope.generateUidIdentifier(node.param.name);

This comment has been minimized.

@bakkot

bakkot Jul 21, 2017

Contributor

node.param.name is presumably always null - I think you instead want a string as the argument to generateUidIdentifier, like "unused".

return;
} else {
const uid = scope.generateUidIdentifier(node.param.name);
node.param.name = uid.name;

This comment has been minimized.

@jridgewell

jridgewell Jul 21, 2017

Member

node.param should be null, so setting param.name will throw an error.

@hzoo

This comment has been minimized.

Member

hzoo commented Jul 23, 2017

Looks good! I would add a change to babel-generator to output a param-less clause - basically when it becomes part of the language we'll want to print it out as is instead transform it so babel should be able to handle that.

Also I would add an exec test to just test that it runs correctly.

visitor: {
CatchClause(path) {
if (path.node.param.name) {

This comment has been minimized.

@jridgewell

jridgewell Jul 23, 2017

Member

I don't understand how this isn't throwing an error. If path.node.param is null (as babylon now sets), this is a nil access.

@@ -32,7 +32,7 @@
"babel-template": "7.0.0-alpha.15",
"babel-traverse": "7.0.0-alpha.15",
"babel-types": "7.0.0-alpha.15",
"babylon": "7.0.0-beta.15",
"babylon": "7.0.0-beta.17",

This comment has been minimized.

@hzoo

hzoo Jul 24, 2017

Member

👍 , you'll need to add it to a few more places (you can just search "babylon": but a reference PR is something like https://github.com/babel/babel/pull/5889/files

@@ -0,0 +1,64 @@
# babel-plugin-transform--optional-catch-binding

This comment has been minimized.

@hzoo

hzoo Jul 24, 2017

Member

extra - here

return true;
}
}
assert(test2());

This comment has been minimized.

@hzoo

hzoo Jul 24, 2017

Member

you want to assert(test()) too right?

This comment has been minimized.

@MarckK

MarckK Jul 25, 2017

Member

lol, yes, accidental deletion.

@hzoo

This comment has been minimized.

Member

hzoo commented Jul 24, 2017

looks good so far, basically done?

We also want to do

https://github.com/babel/babel/wiki/Adding-a-new-Proposal-to-Babel

Can use also https://github.com/babel/babel/pull/5813/files as a reference to files/structure/etc

@jridgewell

Just have @hzoo's nits.

@jridgewell

Oh, sorry, still need babel-generator updates.

@existentialism

@MarckK nice work!

visitor: {
CatchClause(path) {
if (path.node.param) {
return;

This comment has been minimized.

@jridgewell

jridgewell Jul 25, 2017

Member

Nit: this branch can be dropped if we ! the test.

@hzoo

hzoo approved these changes Jul 25, 2017

Nice work!! Great example of a good PR that implements the parser/transform/types/generator/docs change that is required for a proposal to be in Babel. I think the only thing we didn't do is add it to a stage preset since it hasn't even been presented yet 😅

@existentialism existentialism added this to the Babel 7 milestone Jul 25, 2017

@existentialism existentialism merged commit 9fc910d into babel:7.0 Jul 25, 2017

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/project 85.79% (target 80%)
Details

@MarckK MarckK deleted the MarckK:optional-catch-binding branch Jul 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment