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

More robust get constructor name #11

Closed
wants to merge 1 commit into from
Closed

More robust get constructor name #11

wants to merge 1 commit into from

Conversation

lucasfcosta
Copy link
Member

@lucasfcosta lucasfcosta commented Oct 3, 2016

Depends on #9.

Due to chaijs/chai#813 tests in this module started failing because of poorly constructed errors (chaijs/chai#45).

This change also improves detection to whether we've got to run the polyfill or not, because if something already has a .name we can get it instantly.

Also, the new if clause inside getConstructorName guarantees we will be using the result of getFunctionName(new errorLike()) only when it's useful, otherwise the empty string is correct, since we're dealing with an anonymous function.

This was the most concise and simplest way me and @vieiralucas could get to a solution.
Also, special thanks to him for pairing with me to solve this 😄

PS.: WORKS ON IE 😎

@vieiralucas
Copy link
Member

We really need to extract this into it's own module 😸

@keithamus
Copy link
Member

Yeah I feel like if we're going to this much effort to fix this up, let's push it into a separate mini-module. Otherwise though, good changes!

@keithamus
Copy link
Member

@lucasfcosta I've created https://github.com/chaijs/get-function-name, if you would like to create a PR to that repo, that'd be swell. Otherwise if I get some time tonight, I'll do it.

@lucasfcosta
Copy link
Member Author

@keithamus Yes! Great idea.
It would be great since it can get pretty annoying to keep things in sync between Chai and the Check-Error repo.
Also, we can avoid introducing errors into a module whenever changing code on the other one.

@lucasfcosta
Copy link
Member Author

lucasfcosta commented Oct 4, 2016

I'm moving this there. But let get this merged anyway since it has changes to getConstructorName too.
As soon as that new module gets pushed out we can start using it in check-error and Chai.

EDIT: This is done. Let's merge chaijs/get-func-name#1 and generate saucelabs tokens for release.

@meeber
Copy link

meeber commented Oct 16, 2016

LGTM

@vieiralucas
Copy link
Member

vieiralucas commented Oct 16, 2016

Can't we merge only #12 and close this?

@meeber
Copy link

meeber commented Oct 16, 2016

Yup either way works.

Edit: Yeah, let's close this one and merge the other; the commits don't have the same hash id.

@keithamus keithamus closed this Oct 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants