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 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
Copy link
Collaborator

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

@@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@andrewburgess andrewburgess Dec 4, 2019

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

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
@kibertoad
Copy link
Collaborator

thanks!

@kibertoad
Copy link
Collaborator

Released in 0.20.4

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.

2 participants