Skip to content
This repository has been archived by the owner on Aug 4, 2021. It is now read-only.

Fix optional catch binding #70

Merged
merged 3 commits into from
Sep 13, 2019
Merged

Fix optional catch binding #70

merged 3 commits into from
Sep 13, 2019

Conversation

styfle
Copy link
Contributor

@styfle styfle commented Sep 13, 2019

This fixes the scenario where the catch clause does not have a parameter defined.

For example:

try {
  console.log('foo')
} catch {
  console.log('bar')
}

This feature moved to stage 4 last year and is supported since Node 10 and Chrome 66.

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Happy to get this merged asap, thanks for the PR.

@@ -100,7 +100,7 @@ const attachScopes: AttachScopes = function attachScopes(ast, propertyName = 'sc
if (node.type === 'CatchClause') {
newScope = new Scope({
parent: scope,
params: [node.param],
params: node.param ? [node.param] : undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this not be the empty array? Just seems like it would need a lot of guards for the params: undefined case, but if that is already handled then that is ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@guybedford I have no idea. This prevented the code from throwing TypeError: Cannot read property 'type' of null when I changed it.

I was following the example above for BlockStatement which does not assign params.

If you think an empty array is better, that seems to work too.

Copy link
Member

Choose a reason for hiding this comment

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

By the current implementation, it does not really matter what you do, but again I would love to see a test to make sure this keeps working in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lukastaegert Thanks, I added a test 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this null instead of undefined to match the ESTree approach then?

Copy link
Contributor Author

@styfle styfle Sep 13, 2019

Choose a reason for hiding this comment

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

@guybedford It can't be null due to the interface:

interface ScopeOptions {
	parent?: AttachedScope;
	block?: boolean;
	params?: Array<Node>;
}

It can only be undefined or Array. I'll change it to [].

Copy link
Member

@lukastaegert lukastaegert left a comment

Choose a reason for hiding this comment

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

Test would be nice

@styfle
Copy link
Contributor Author

styfle commented Sep 13, 2019

@lukastaegert I added a test but Windows CI is failing with the following:

ERR! Error: EPERM: operation not permitted, unlink 'C:\projects\rollup-pluginutils\node_modules\.staging\typescript-5413e3f3\lib\pl\diagnosticMessages.generated.json'

@styfle
Copy link
Contributor Author

styfle commented Sep 13, 2019

It looks like pushing again solved the CI issue ✅

@guybedford guybedford merged commit c94af05 into rollup:master Sep 13, 2019
@guybedford
Copy link
Contributor

Released in 2.8.2.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants