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)) {

This comment has been minimized.

@joepie91

joepie91 Mar 14, 2017
Contributor

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.

This comment has been minimized.

@wubzz

wubzz Mar 14, 2017
Author Member

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
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants