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

Fix queryContext function defintion #3562

Merged
merged 2 commits into from Dec 4, 2019
Merged

Fix queryContext function defintion #3562

merged 2 commits into from Dec 4, 2019

Conversation

@andrewburgess
Copy link
Contributor

andrewburgess commented Dec 4, 2019

According to the docs, calling queryContext with no arguments returns the context object. (ref: https://knexjs.org/#Builder-queryContext)

This PR adds the relevant overload to the function definition for that use case.

According to the docs, calling `queryContext` with no arguments returns the context object.

Add the relevant overload to the function definition for that use case.
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Dec 4, 2019

@@ -1330,6 +1330,7 @@ declare namespace Knex {
wrap<TResult2 = TResult>(before: string, after: string): Raw<TResult>;
toSQL(): Sql;
queryContext(context: any): Raw<TResult>;
queryContext(): any;

This comment has been minimized.

Copy link
@kibertoad

kibertoad Dec 4, 2019

Collaborator

would it be better defined as Record<string, any>?

This comment has been minimized.

Copy link
@andrewburgess

andrewburgess Dec 4, 2019

Author Contributor

I'm not sure, would that be for the return type?

From what I can tell it returns exactly what was put in when called with an argument, so I figured it would be best to just have those types match

This comment has been minimized.

Copy link
@kibertoad

kibertoad Dec 4, 2019

Collaborator

ok, fair :). does it work correctly if user passes e. g. string?

This comment has been minimized.

Copy link
@andrewburgess

andrewburgess Dec 4, 2019

Author Contributor

Yep, this is the underlying implementation:

knex/lib/helpers.js

Lines 79 to 85 in 988fb24

Target.prototype.queryContext = function(context) {
if (isUndefined(context)) {
return this._queryContext;
}
this._queryContext = context;
return this;
};

And the docs say the queryContext is shallowly cloned when a query builder is cloned:

cloned._queryContext = clone(this._queryContext);

which uses lodash.clone and should work on any value passed in

@andrewburgess

This comment has been minimized.

Copy link
Contributor Author

andrewburgess commented Dec 4, 2019

Thanks! Could you also update https://github.com/knex/knex/blob/master/types/test.ts?

Can do

@kibertoad kibertoad merged commit 2289c08 into knex:master Dec 4, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Dec 4, 2019

thanks!

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Dec 7, 2019

Released in 0.20.4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.