WIP: Add stricter Query types

[marionebl]

• Sync on testing approach
• Create a basic type test suite for QueryBuilder and Query
• Implement stricter types

This change is 

[pkaminski]

Thanks for working on this! Let's use Reviewable for code reviews if you don't mind -- it's my own project that pays the bills, so I'm kinda partial to it. 😃

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @marionebl)

---

package.json, line 28 at r1 (raw file):

  },
  "tsd": {
    "directory": "test-d",

Would it make sense to call this directory type-tests to match the script and the naming pattern for the normal tests directory, or is test-d the convention for this tool? (Another suggestion might be tests-d.)

---

test-d/query.test-d.ts, line 4 at r1 (raw file):

import {System} from '../src/system';

test('query has not flavours by default', () => {

Nit: not -> no, flavours -> flavors (throughout).

---

test-d/query.test-d.ts, line 4 at r1 (raw file):

import {System} from '../src/system';

test('query has not flavours by default', () => {

Knowing nothing about tsd, should these tests be grouped into suites with describe or some such?

---

test-d/query.test-d.ts, line 20 at r1 (raw file):

  class UnusedTestSystem extends System {
    private q = this.query(b => b.with(A));

Incidentally, having a with or without clause in a flavorless query should probably be invalid -- only using makes sense in this case. I'll need to add this check at runtime.

---

test-d/query.test-d.ts, line 39 at r1 (raw file):

    private q = this.query(b => b.with(A));
    execute() {
      expectType<ExpectedShape>(this.q);

I assume this test (and the ones below) don't currently pass? A .with will not add a current flavor all by itself.

---

test-d/query.test-d.ts, line 50 at r1 (raw file):

  interface ExpectedShape {
    current: readonly A[];

A query's properties will always be lists of entities, not lists of components, so I'm not sure how this is intended to work. Ditto below.

[marionebl]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @pkaminski)

---

package.json, line 28 at r1 (raw file):

Previously, pkaminski (Piotr Kaminski) wrote…

Would it make sense to call this directory type-tests to match the script and the naming pattern for the normal tests directory, or is test-d the convention for this tool? (Another suggestion might be tests-d.)

test-d is in fact the default, so this config isn't strictly required. I think it makes sense to call it type-tests as per your suggestion. I'll change it to be consistent.

---

test-d/query.test-d.ts, line 4 at r1 (raw file):

Knowing nothing about tsd, should these tests be grouped into suites with describe or some such?

I think there is no convention established around this as tsd is a pretty nice tool. The test function wrappers I use here are not used by the tool in any way, I just use them to provide a description.

We could achieve the same thing via comments if you prefer, although I like the logical grouping the current variant established.

---

test-d/query.test-d.ts, line 4 at r1 (raw file):

Previously, pkaminski (Piotr Kaminski) wrote…

Nit: not -> no, flavours -> flavors (throughout).

Ok, will fix.

---

test-d/query.test-d.ts, line 39 at r1 (raw file):

Previously, pkaminski (Piotr Kaminski) wrote…

I assume this test (and the ones below) don't currently pass? A .with will not add a current flavor all by itself.

Yes, all the tests don't pass - they are intended as means to sync about expectations between the two of us at this point.

---

test-d/query.test-d.ts, line 50 at r1 (raw file):

Previously, pkaminski (Piotr Kaminski) wrote…

A query's properties will always be lists of entities, not lists of components, so I'm not sure how this is intended to work. Ditto below.

Ok, will fix.

[marionebl]

---

test-d/query.test-d.ts, line 4 at r1 (raw file):

Previously, marionebl (Mario Nebl) wrote…

Knowing nothing about tsd, should these tests be grouped into suites with describe or some such?

I think there is no convention established around this as tsd is a pretty nice tool. The test function wrappers I use here are not used by the tool in any way, I just use them to provide a description.

We could achieve the same thing via comments if you prefer, although I like the logical grouping the current variant established.

nice niche

[pkaminski]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @marionebl and @pkaminski)

---

test-d/query.test-d.ts, line 4 at r1 (raw file):

Previously, marionebl (Mario Nebl) wrote…

nice niche

I like the test functions, but I take it that tsd won't invoke them automatically? If so, would it be possible to put these tests under Jest's control instead (in tests/) and just use tsd as a custom assertion provider? Their docs allude to using Jest but don't really explain how to make it work.

---

test-d/query.test-d.ts, line 39 at r1 (raw file):

Previously, marionebl (Mario Nebl) wrote…

Yes, all the tests don't pass - they are intended as means to sync about expectations between the two of us at this point.

OK, then my expectation is that this test shouldn't pass, since current doesn't get added in by default. 😄