Typings revised for passing result type down the query builder chain #744

jtlapp · 2018-01-22T00:57:53Z

Typings revision per our conversation, @mceachen, for #742. Also had to prettier it up.

I think we left one topic unresolved: defaulting QueryBuilder<M> to yielding an M[] instead of an M, as things stand.

Not the problem: I think the main problem with changing it to M[] is that, because we can only default unspecified parameters on the right, we'd end up specifying all the parameters QueryBuilder<RM, RV, QB> or QueryBuilder<RM, QB, RV> in many typings occurrences that are now just singlets or couplets. Putting RV last would reduce that some, but it might be a bit confusing to see the type in the form QueryBuilder<return, queried, return>, since the return types aren't adjacent.

Possible problem: We can infer M[] from M but not M from M[].

UPDATE: I created an experimental branch offshoot of this PR that defaults to Model[], but I don't have $relatedQuery() working.

coveralls · 2018-01-22T01:01:54Z

Coverage remained the same at 95.041% when pulling 93fe100 on jtlapp:pass-thru-typings into 259b6ee on Vincit:master.

coveralls · 2018-01-22T01:01:54Z

Coverage remained the same at 95.041% when pulling 93fe100 on jtlapp:pass-thru-typings into 259b6ee on Vincit:master.

coveralls · 2018-01-22T01:01:54Z

Coverage remained the same at 95.041% when pulling 93fe100 on jtlapp:pass-thru-typings into 259b6ee on Vincit:master.

coveralls · 2018-01-22T01:01:54Z

Coverage increased (+0.02%) to 95.066% when pulling 449ba84 on jtlapp:pass-thru-typings into 259b6ee on Vincit:master.

koskimas · 2018-01-22T12:02:37Z

tests/ts/examples.ts

-qb = qb.runBefore(async (result: any, builder: objection.QueryBuilder<Person>) => {});
-qb = qb.runAfter(async (result: Person[], builder: objection.QueryBuilder<Person>) => {});
+qb = qb.runBefore(async (result: any, builder: objection.QueryBuilder<Person, Person[]>) => {});
+qb = qb.runAfter(async (result: Person[], builder: objection.QueryBuilder<Person, Person[]>) => {});


One should return stuff from these. Whatever you return from runBefore gets passed to the next runBefore function as the first argument. That's impossible write typings for, but return value should be required.

Whatever you return from runAfter ends up being the return value of the query (in addition to being the input for the next runAfter). This we probably could write typings for?

I wondered about that. I'll see what I can do.

Okay, it seems that the purpose of these methods is to allow code to do an out-of-order prepending or appending of query actions to the query chain.

If the query chain were written expecting runBefore to change the result type that the first query method acts on, the type for the first method in the chain would be wrong, and the dev would have to typecast it. The typings only work if the final output of the runBefore series has the same type as the input to the first query method, which is now Model | Model[].

I'm unable to find a way to incorporate runAfter result types. It would require adding a fourth type parameter to QueryBuilder and passing it from query builder to query builder through the typings. Call this type AV. We'd want execute to yield type AV when defined and RV otherwise, but AFAIK, Typescript cannot conditionally select among types. We could initialize AV to never so that RV | AV evaluates to RV, but RV | AV would never evaluate to just AV because RV would never be never.

Unless there's a Typescript trick for doing this that I'm unaware of.

koskimas · 2018-01-22T12:04:21Z

tests/ts/examples.ts

+const findOneWhereInThrow: Promise<Person> = Person.query()
+  .findOne('raw sql')
+  .whereIn('status', ['active', 'pending'])
+  .throwIfNotFound();


You cannot give a string for findOne. Also none of the methods accept raw sql as a string. Either objection.raw or knex.raw needs to be used.

I don't know if it should be explicitly prevented, but having these examples is misleading.

I'll change the findOne() parameters to something valid. They don't matter for the purpose of this test, so might as well pick something that's possible.

Corrected in the new push.

koskimas · 2018-01-22T12:07:57Z

typings/objection/index.d.ts

+  type ColumnRef = string | Raw | Reference | QueryBuilder<any, any[]>;
+  type TableName = string | Raw | Reference | QueryBuilder<any, any[]>;
+
+  interface QueryInterface<QM, RM, RV> {


I know QueryInterface was not added in this commit, but I've always wondered what's the purpose of it? Only one class ever implements it. Couldn't we just move these methods over to QueryBuilder and eliminate the whole interface?

Are the QueryInterface methods all borrowed from Knex, while the QueryBuilder methods added by Objection? If so, it might help with maintenance to keep them separate. But then we might want to rename QueryInterface to make that clearer.

I do find the names QueryBuilder and QueryBuilderBase confusing. Both names suggest that they're the top-most query builder interface, but QueryBuilder extends QueryBuilderBase. At least in this new merging of query builders, every QueryBuilder instance implements QueryBuilderBase, so we could merge these two interfaces.

I believe QueryInterface and QueryBuilder could be coalesced.

With respect to the name, when I'd initially done the typings, I had to copy over the knex typings for QueryBuilder/QueryInterface into objection to extend them with generics (and I still feel bad about that). I've manually synced them a couple times, but I'd love to not need to do that.

What would be ideal is if this copypasta could go away, and use the knex typings directly, but that would require the diff I'd made be incorporated into the knex typings. @koskimas do you think that'd be a welcome change?

@mceachen, what to you think of combining QueryBuilder and QueryBuilderBase, per my comment above?

If you can pull it off and not break existing tests, go for it: less is more.

I got answers. The QueryBuilder and QueryBuilderBase types may correspond to classes of those names in the implementation. If these types do indeed correspond to these classes, we may want to keep the names the same to facilitate maintenance.

OTH, as a person perusing just the public API -- including the typings -- I think I'd prefer calling QueryBuilderBase and QueryBuilder by the names of AbstractQueryBuilder and QueryBuilder, to tell me their roles and relationship at a glance. (QueryBuilder could be the interface for QueryBuilderBase, or QueryBuilderBase could be the base (class?) of QueryBuilder. It's ambiguous to me.)

Also, I was unable to merge these two interfaces because the return types of throwIfNotFound() cannot be generalized more strictly than any.

I did however move returning() and runAfter() to QueryBuilderBase. I think runAfter() may have originally been query-builder-specific in order to map the output of the query builder to the input of runAfter(). But because runAfter() callbacks can be chained, we can't hardcode its input type. I changed it to any.

koskimas · 2018-01-22T12:15:45Z

tests/ts/examples.ts

+const deleteByIDThrow: Promise<number> = Person.query()
+  .deleteById(32)
+  .throwIfNotFound();
+


Are there tests for these cases:

// first turns array result into single const x1: Promise<Model> = qb.where().first(); // the location of `first` doesn't matter const x2: Promise<Model> = qb.first().where(); // returning turns `number` result back to `M[]` const x3: Promise<Model[]> = qb.where().patch().returning(); // the position of `returning` doesn't matter (as long as it comes after the method that turns type into non-model) const x4: Promise<Model> = qb.delete().returning().first().where();

I'll check and add them if not. I want more returning() test cases as well. We put returning() on the base query builder, so it's always accessible.

I'm noticing that my new examples are only testing the static methods. I wonder if I should duplicate them all for the instance methods.

I'd tried to hit both in the past. If you could add them, that'd be great.

Added them in the new push, plus a few more of the returning() variety, as well as their static method equivalents.

All tests continue to pass.

BTW, your x1, x2, and x4 tests should return Promise<Model | undefined>.

I'm thinking I should duplicate all my static method tests as instance method tests. I think it has to be code duplication because the testing occurs at tsc compile time.

Yeah, you're right, it has to be duplicated code. Perhaps adding a comment to each section saying why the duplication is happening will somewhat appease the Engineers of Tomorrow when they judge us for our code duplication sins?

appease the Engineers of Tomorrow when they judge us for our code duplication sins?

LOL! Will do!

Maybe we don't need to dup many tests. After the first call to Model.query() or model.$query(), we're testing the same query builder methods. I'm thinking that we can provide coverage either way and then throw in a few token tests for other methods of getting query builders. The only internals we're taking advantage of is our knowing that they're all using the same query builder types.

Less code duplication makes for easier maintenance. And less judging! What do you think?

Whatever you do, I'd suggest leaving a comment in the code explaining why there is either code duplication or a lack of duplicated instance/static tests. I think that will cover our butts from future judging.

In general, I'll always lean towards less code, if we can get away with it.

jtlapp · 2018-01-22T21:40:40Z

tests/ts/examples.ts

+  .runAfter(async (result: any, builder: objection.QueryBuilder<Person>) => 88);
+const runAfterPersons: Promise<Person[]> = qb.runAfter(
+  async (result: any, builder: objection.QueryBuilder<Person, Person[]>) => 88
+);



@koskimas, perhaps you can review these runBefore/runAfter tests? I'm mainly ensuring that these methods do not affect the query builder type chain, at least not in-place.

jtlapp · 2018-01-22T21:42:35Z

tests/ts/examples.ts

+  let instanceQB = person.$query();
+  instanceQB = staticQB;
+}
+


@mceachen, I think I managed to avoid code duplication entirely, as well as make no assumptions about implementation. What do you think of this?

fixes Vincit#748

jtlapp · 2018-01-23T01:09:11Z

@mceachen, I experimented with defaulting query builders to using Model[], and the only thing I don't have working is $relatedQuery(). I figured that's your baby, you'd want to trouble-shoot it.

I made a model-array-default branch off of the pending pass-thru-typings PR. For your convenience:

mceachen · 2018-01-23T06:15:17Z

@koskimas I'll have time to review this tomorrow, can you hold off on releasing 1.0.0 until tomorrow afternoon? It'd be nice if we got this refactoring into 1.0.0.

jtlapp · 2018-01-23T12:08:51Z

Also, I may have a way to improve type support of model fields -- using partials in cases where models are currently typed whole but are actually partially returned, not using partials in cases where they are currently typed partial when build-time-required model fields are actually required.

I might be able to do it in a backward-compatible way, but it's not entirely clear to me yet.

It would be nice to hold off on 1.0 to let me continue working with the typings without feeling hamstrung for backward-compatibility. I'm racing as fast as I can.

cc @koskimas, @mceachen

UPDATE: And I think I can provide most of the custom query builder typings solution, only requiring the user to do a little work that Typescript generics won't provide out-of-the box.

Here are examples of the Partial issues that I think I can fix:

class Person extends Model {
  firstName: string;
  lastName: string;
  age?: number;

  static get jsonSchema () {
    return {
      type: 'object',
      required: ['firstName', 'lastName'],
    };
  }
}

// This should error at build time because lastName is missing.*
Person.query().insert({firstName: 'Mo'});

Person.query().update({firstName: 'Mo'}).returning('age').then(peeps => {
  // This should warn at build time that lastName may be undefined.**
  const lastName: string = peeps[0].lastName;
});

* - My experiments show that I can make this behave the way it currently does by default.
** - I'm looking at either returning a Partial of the model or constructing the returned value from the returning() keys and possibly also the provided keys. I don't think I can default the behavior to its current form. This could force some existing code to have to insert bangs to disable the warning.

These latter mods should be a new PR -- after this one -- so we can evaluate them separately.

mceachen · 2018-01-23T18:19:05Z

@jtlapp I've been trying out this PR against my personal project, and the only issue I have is QueryBuilder's default of RV=RM, I really think it should be RV=RM[]. .query(), for example, returns all rows (and should, arguably, drive the default). (I think it's telling that there's 30 references to QueryBuilder<QM, QM[]>--that also suggests it should be the default).

I also tried removing the RM, and rediscovered what you'd found that it was required to make .returning and like recover the return value, so I think that's defensible.

@koskimas, @jtlapp do you think it's reasonable to make QueryBuilder default return an array?

mceachen · 2018-01-23T18:29:56Z

I'm also trying to flip the RM, RV generics so you don't have to do <Person, Person, Person[]> which makes me sad.

jtlapp · 2018-01-23T18:57:44Z

@mceachen, I have all that fixed in the model-array-default branch, per my above comment. The only thing not backward-compatible in that branch is $relatedQuery(), which I think is not quite working as you expect even in the current master.

jtlapp · 2018-01-23T19:01:19Z

And yes, I agree that the default should be a model array, given that you have to go out of your way in the query builder chain to get anything else. The only drawback is that you have to do the unexpected parameterization QueryBuilder<Model, Model> to get a single model out -- it just looks redundant.

jtlapp · 2018-01-23T19:10:45Z

If you want, I can push the model array default solution to this PR, so that we're evaluating that or no pass-through-result-types at all.

…nals

jtlapp · 2018-01-23T23:00:55Z

I attempted to merge in @mceachen's mods but ended up screwing up the PR. #753 replaces this PR.

jtlapp · 2018-01-23T23:16:47Z

@mceachen and I discussed supporting partials. It seems like a reasonable thing to do, but possibly non-trivial to do right. We're punting for 1.0. I think we're done with this investigation.

jtlapp added 3 commits January 21, 2018 16:16

revised typings to pass result type along query builder chain

f63b74c

Combined returning() into top QueryBuilder

df26e3e

prettied up result-pass-thru typings

93fe100

made non-throwIfNotFound() versions of tests not having them

7ee4228

koskimas reviewed Jan 22, 2018

View reviewed changes

koskimas and others added 7 commits January 22, 2018 15:40

separate Selection class from SelectOperation.js

d6d818c

rename Selection.selects --> includes

0b90f6a

add mention about private API in API.md

29c3481

added first()-order typings tests and returning() tests

a8f0d15

fixed runBefore() and runAfter() types, added more tests

a15cf85

confirms equiv of query() and () so don't have to dup tests

fd461a2

prettied runBefore/runAfter and query/ tests

332e4bb

jtlapp commented Jan 22, 2018

View reviewed changes

jtlapp and others added 8 commits January 22, 2018 16:14

moved returning() and runAfter() to QueryBuilderBase

ce7689d

move db metadata querying from RelationJoinBuilder to Model

c16aaae

fixes Vincit#747

43ec07b

Fix parsing relationships in case source data is already Model instance

b57d1bf

fixes Vincit#748

v1.0.0-rc.5

5451ff4

generalized runBefore/runAfter builder arg result type

a8b28e6

QBs default to Model[], but relatedQuery not working

3f09ca5

removed explicit type-paraming result to Model[]

f838f14

koskimas added 4 commits January 23, 2018 14:19

fixes Vincit#750

f305d88

fixes Vincit#745

961e6b4

add error handling recipe

2108337

better error handling recipe

b58a6bb

jtlapp and others added 8 commits January 23, 2018 15:24

removed inferred $relatedQuery() typing and associated examples

5a84d81

new typings header provided by @mceachen

055b025

Merge remote-tracking branch 'joe/pass-thru-typings' into pr_744_try_2

f519c45

Merge remote-tracking branch 'joe/model-array-default' into pr_744_try_2

f3ee8f9

add filter example

85f0d87

add keyof types to where clause to add IDE help when writing conditio…

aa69d9d

…nals

add $relatedQuery workaround

65ba2f5

add note and more examples

449ba84

mceachen mentioned this pull request Jan 23, 2018

Pr 744 try 2 #753

Merged

jtlapp closed this Jan 23, 2018

mceachen mentioned this pull request Jan 25, 2018

QueryBuilder.relate() and .patch() yield the wrong type #717

Closed

Wardormeur mentioned this pull request Mar 14, 2018

Count doesn't return a number when used in subquery #840

Closed

Typings revised for passing result type down the query builder chain #744

Typings revised for passing result type down the query builder chain #744

Conversation

jtlapp commented Jan 22, 2018 • edited Loading

coveralls commented Jan 22, 2018

coveralls commented Jan 22, 2018

coveralls commented Jan 22, 2018

coveralls commented Jan 22, 2018 • edited Loading

koskimas Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtlapp Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koskimas Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mceachen Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtlapp Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtlapp commented Jan 23, 2018

mceachen commented Jan 23, 2018

jtlapp commented Jan 23, 2018 • edited Loading

mceachen commented Jan 23, 2018 • edited Loading

mceachen commented Jan 23, 2018

jtlapp commented Jan 23, 2018 • edited Loading

jtlapp commented Jan 23, 2018

jtlapp commented Jan 23, 2018

jtlapp commented Jan 23, 2018

jtlapp commented Jan 23, 2018

jtlapp commented Jan 22, 2018 •

edited

Loading

coveralls commented Jan 22, 2018 •

edited

Loading

koskimas Jan 22, 2018 •

edited

Loading

jtlapp Jan 22, 2018 •

edited

Loading

koskimas Jan 22, 2018 •

edited

Loading

mceachen Jan 22, 2018 •

edited

Loading

jtlapp Jan 22, 2018 •

edited

Loading

jtlapp commented Jan 23, 2018 •

edited

Loading

mceachen commented Jan 23, 2018 •

edited

Loading

jtlapp commented Jan 23, 2018 •

edited

Loading