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

Transaction should not reject with undefined. #1970

Merged

Conversation

wubzz
Copy link
Member

@wubzz wubzz commented Mar 14, 2017

No description provided.

@wubzz wubzz mentioned this pull request Mar 14, 2017
this._resolver(value)
}
if (status === 2) {
if(isUndefined(value)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the intention to only do this when rejecting with undefined? I'm not sure under what circumstances this issue could occur (as I haven't followed the issue closely), but other non-Error rejection values such as numbers or strings would have the same "missing stacktrace" issue.

Copy link
Member Author

@wubzz wubzz Mar 14, 2017

Choose a reason for hiding this comment

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

Yes, this is intentional. By definition undefined is the only javascript type that can be considered non-existant. Null, Strings, Numbers, Objects, Errors, Buffers, or any other type of value is still valid.

For example some people prefer to use strings (why is beyond me):

try {
    throw 'poop'
} catch(e) {
    console.log(e, typeof e);
    //poop string
}

So in cases where rollback is called with an explicit value other than undefined, that value will be used in the reject handler.

@wubzz wubzz merged commit b722753 into knex:master Mar 14, 2017
elhigu added a commit that referenced this pull request Mar 26, 2017
* Fixed transaction promise to not be overridden

* Disabled failing test case until #1970 feature if fixed.
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

2 participants