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

Fix error on promise .and() #428

Merged
merged 4 commits into from Nov 20, 2017

Conversation

Projects
None yet
3 participants
@alexjeffburke
Collaborator

alexjeffburke commented Nov 13, 2017

Closes #429

@alexjeffburke

This comment has been minimized.

Show comment
Hide comment
@alexjeffburke

alexjeffburke Nov 13, 2017

Collaborator

I think this commit should address the promise issue from earlier today.

Note, grepping around I found that callInNestedContext has similar looking code but I'd need someone much more familiar with it to tell me if it's relevant there too.

Collaborator

alexjeffburke commented Nov 13, 2017

I think this commit should address the promise issue from earlier today.

Note, grepping around I found that callInNestedContext has similar looking code but I'd need someone much more familiar with it to tell me if it's relevant there too.

@alexjeffburke alexjeffburke requested a review from papandreou Nov 15, 2017

@papandreou

Looks great, just a few minor nits.

Yeah, I'm fairly sure that a similar treatment is needed in a bunch of other places. Could be interesting to try to see if a similar error can be provoked from other angles.

In any case, this is a very useful fix on its own, so there's no reason to expand the scope.

Show outdated Hide outdated test/api/shift.spec.js Outdated
Show outdated Hide outdated test/api/shift.spec.js Outdated
Show outdated Hide outdated lib/Unexpected.js Outdated
@alexjeffburke

This comment has been minimized.

Show comment
Hide comment
@alexjeffburke

alexjeffburke Nov 17, 2017

Collaborator

@papandreou I’ve pushed changes that should address all of your review comments. I took the opportunity to do what we discussed around entirely replacing isPromisePending and did manage to reproduce the issue in the other place - have included that test too :)

Collaborator

alexjeffburke commented Nov 17, 2017

@papandreou I’ve pushed changes that should address all of your review comments. I took the opportunity to do what we discussed around entirely replacing isPromisePending and did manage to reproduce the issue in the other place - have included that test too :)

@papandreou

Awesome 🚀

Thanks for sorting all this out!

@alexjeffburke

This comment has been minimized.

Show comment
Hide comment
@alexjeffburke

alexjeffburke Nov 19, 2017

Collaborator

@sunesimonsen If you're comfortable with this too I'll go ahead and merge :)

Collaborator

alexjeffburke commented Nov 19, 2017

@sunesimonsen If you're comfortable with this too I'll go ahead and merge :)

@sunesimonsen

This comment has been minimized.

Show comment
Hide comment
@sunesimonsen

sunesimonsen Nov 20, 2017

Member

@alexjeffburke @papandreou approval for anything that is not controversial should be enough.

Member

sunesimonsen commented Nov 20, 2017

@alexjeffburke @papandreou approval for anything that is not controversial should be enough.

@sunesimonsen

👍

@alexjeffburke alexjeffburke merged commit af56da0 into master Nov 20, 2017

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.004%) to 98.141%
Details
@alexjeffburke

This comment has been minimized.

Show comment
Hide comment
@alexjeffburke

alexjeffburke Nov 20, 2017

Collaborator

Released as Unexpected v10.36.2.

Collaborator

alexjeffburke commented Nov 20, 2017

Released as Unexpected v10.36.2.

@alexjeffburke alexjeffburke deleted the fix/errorOnPromiseAnd branch Nov 20, 2017

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