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

assert: support arrow functions in .throws() #3276

Merged
merged 1 commit into from
Oct 8, 2015

Conversation

bnoordhuis
Copy link
Member

x instanceof f where f is an arrow function throws a (spec-conforming)
"Function has non-object prototype 'undefined' in instanceof check"
exception.

Add a workaround so that it's possible to pass arrow functions as the
second argument to assert.throws(). The try/catch block is a little
jarring but swapping around the clauses in the if statements changes
the semantics too much.

Fixes: #3275

R=@thefourtheye

CI: https://ci.nodejs.org/job/node-test-pull-request/453/

@thefourtheye
Copy link
Contributor

LGTM. Two weeks back I faced the same problem and tried the exact same fix locally :D

@targos
Copy link
Member

targos commented Oct 8, 2015

Isn't it enough to do if(expected.prototype && actual instanceof expected) if you don't want the try/catch ?

@thefourtheye
Copy link
Contributor

@targos Wouldn't it be better if we let v8 take care of it?

@bnoordhuis
Copy link
Member Author

What @thefourtheye said. I'm inclined to be reactive here, not proactive. An explicit check like that can be subverted.

return true;
}
} catch (e) {
// Ignore. The instanceof check doesn't work for arrow functions.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: there are two white spaces between both sentences.

Copy link
Member Author

Choose a reason for hiding this comment

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

You may have noticed that I do that in most places. It's called double spacing and religious wars have been fought over whether or not it's a good thing.

Copy link

Choose a reason for hiding this comment

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

Double spacing is spacing between lines... I think you guys are referring to the spaces after the period, in the comment? (maybe not; then please ignore this comment)

@targos
Copy link
Member

targos commented Oct 8, 2015

OK then LGTM

@mscdex mscdex added the assert Issues and PRs related to the assert subsystem. label Oct 8, 2015
`x instanceof f` where f is an arrow function throws a (spec-conforming)
"Function has non-object prototype 'undefined' in instanceof check"
exception.

Add a workaround so that it's possible to pass arrow functions as the
second argument to assert.throws().  The try/catch block is a little
jarring but swapping around the clauses in the if statements changes
the semantics too much.

Fixes: nodejs#3275
PR-URL: nodejs#3276
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@bnoordhuis bnoordhuis closed this Oct 8, 2015
@bnoordhuis bnoordhuis deleted the bug3275 branch October 8, 2015 17:29
@bnoordhuis bnoordhuis merged commit ded4f91 into nodejs:master Oct 8, 2015
@jasnell
Copy link
Member

jasnell commented Oct 9, 2015

@bnoordhuis ... should this land in v4.x before LTS is cut?

@bnoordhuis
Copy link
Member Author

Good point. Yes, tag added.

bnoordhuis added a commit that referenced this pull request Oct 10, 2015
`x instanceof f` where f is an arrow function throws a (spec-conforming)
"Function has non-object prototype 'undefined' in instanceof check"
exception.

Add a workaround so that it's possible to pass arrow functions as the
second argument to assert.throws().  The try/catch block is a little
jarring but swapping around the clauses in the if statements changes
the semantics too much.

Fixes: #3275
PR-URL: #3276
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jasnell
Copy link
Member

jasnell commented Oct 10, 2015

Landed in v4.x in 8383c4f

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants