edgedb / edgedb-js

The official TypeScript/JS client library and query builder for EdgeDB
https://edgedb.com
Apache License 2.0
514 stars 65 forks source link

Shape with pointer of `false` produces type suggesting the pointer is returned #1037

Closed CarsonF closed 4 months ago

CarsonF commented 5 months ago
e.select(e.X, {
  foo: false
})

Runtime omits foo from EdgeQL, but TS types it as never. This allows it to be compatible, somehow, with a type expecting that property.

class X {
  foo: string;
}
const withX = (x: X) => {...};

const result = await e.select(e.X, {
  foo: false
}).run();

// This is fine even though `foo` is required, and not in the DB result.
withX(result[0]);

I think the type should be changed to undefined, or maybe unknown instead?

scotttrinh commented 5 months ago

Yeah, great point. "never is assignable to anything"

Options:

  1. Remove this key from the resulting object.
  2. Assign undefined to this key in the resulting object and make the type reflect that.
  3. Use unknown and... not sure what the runtime should be here since unknown doesn't really imply anything to me about this key in the resulting object. Other than maybe that it exists?

I think the first option makes the most sense since I think that's kind of how you would instinctively use false: as a way to remove a property (for instance after a ["*"] splat spread).

CarsonF commented 5 months ago

Could we change the arg shape to reject false? https://github.com/edgedb/edgedb-js/blob/0bf404ec6e3479379417b234e0500e2f4b76310d/packages/generate/src/syntax/select.ts#L755-L755

| true
diksipav commented 4 months ago

We fixed this issue with first @scotttrinh recommendation above. We don't return anymore "these keys" in the resulting objects.