feat(types): add embeded functions type inference #632
Conversation
avallete
changed the title
There was a problem hiding this comment.
This one was made by locally buidling: supabase/postgres-meta#971
And running it for introspection.
Then canary tested by passing this as a locally built dependency to supabase-js and using it on our codebase with success.
I think this might introduce some breaking changes though and we might need to make a new major for it. As it happened in the past it's really easy for types changes to break users code, best would be to start with some canary testings and gather feedbacks.
I'm thinking something like:
- Have the new introspection released.
- Ensure that it doesn't break the current type inference (it revert the function introspections to an union of args+returns so this might cause some issues)
- Ensure the current
postgrest-jslatest version is fully released (see: supabase/supabase-js#1491 and supabase/ssr#119 ) - Allow users to opt-in to the new version and make sure there is no big issues.
- Release to the general channel.
| { | ||
| const result = await postgrest.rpc('get_status') | ||
| if (result.error) { | ||
| throw new Error(result.error.message) | ||
| } | ||
| expectType<'ONLINE' | 'OFFLINE'>(result.data) | ||
| } | ||
|
|
There was a problem hiding this comment.
comment
Moved in favor of runtime & type tests: https://github.com/supabase/postgrest-js/pull/632/files#diff-1496003b365eab7067c12b679d21e8db1289c1b1da1a7b36399894d21e0c5ef3R211-R249
| @@ -12,48 +12,50 @@ const postgrestWithOptions = new PostgrestClient<DatabaseWithOptions>(REST_URL) | |||
|
|
|||
| // table invalid type | |||
| { | |||
| expectError(postgrest.from(42)) | |||
| expectError(postgrest.from('nonexistent_table')) | |||
| // @ts-expect-error should be error | |||
There was a problem hiding this comment.
comment
Move away from the expectError to the @ts-expect-error directive.
This give us a better DX since:
- If we're expecting an error which is no more, it pops up into the editor at typing time directly rather than later when running tsd.
- Reduce the noise within the editor/project about errors where they're actually expected.
Note that test:types will still fail if one of those errors disappear while the directive is still in place.
There was a problem hiding this comment.
Note that expectError() checks if the error belongs to the list of known errors. In contrary // @ts-expect-error silences any errors. Can be missing imports, syntax errors or simply typo. Refactoring is often the reason that turns // @ts-expect-error tests into always passing.
The problem is that // @ts-expect-error was not designed to be a testing API.
I think, ideally a type testing tool should be able to check messages provided with // @ts-expect-error. This is how TSTyche (https://tstyche.org) works, that is a type testing tool I am working on.
Even better, there is .not.toBeCallableWith() matcher:
expect(postgrest.from).type.not.toBeCallableWith('nonexistent_table')(These are only attempts to find a better solution for a real world problem.) All I try to say, // @ts-expect-error does not work in long term. Look at this file, imports were not copied while refactoring, but the test keeps passing. The tsds expectError() would catch this.
There was a problem hiding this comment.
Super useful insight ! That was an overlook from my end indeed, I didn't knew expectError actually had a list of ignored errors (TIL 😅)
TSTyche looks great ! I've been looking for something like this for some time !
I think using the directives error messages matching could be a first step toward leveraging it since that's what's currently in most of the tests while keeping a stricter runtime check on it with error messages matchings.
Does that sounds good to you ? If that does I'll do this migration (index-d.test.ts + tstyche setup with checkSuppressedErrors matchings) in a dedicated PR.
There was a problem hiding this comment.
Sounds good! Please ping me, always happy to review.
By the way, you can start with running:
npm create tstyche@latestInclude the following additionally to the tstyche.config.json:
"testFileMatch": ["test/index.test-d.ts"],
"tsconfig": "./tsconfig.test.json"And all is set to play with npx tstyche (;
| import { Database as OriginalDatabase } from './types.generated' | ||
| export type Json = unknown | ||
|
|
||
| export type Database = { | ||
| export type Database = OriginalDatabase & { |
There was a problem hiding this comment.
comment
Reduce the override to the minimum by leveraging the type union.
| Views: {} | ||
| Enums: {} | ||
| CompositeTypes: {} |
There was a problem hiding this comment.
issue
For some reason, not declaring those with empty values now fail the merge with the utils types down the line 🤔
| export type GenericFunction = { | ||
| Args: Record<string, unknown> | ||
| Args: Record<string, unknown> | never |
There was a problem hiding this comment.
comment
The generated function now use the never as argument which allows to distinguish between "no params at all" and "param with empty name".
That should still be retro-compatible since Record<PropertyKey, never> should be considered as Record<string, unknown>.
avallete
force-pushed
the
feat/add-embeded-functions-type-inference
branch
from
b7344cf to
d8b677f
Compare
What kind of change does this PR introduce?
Additional context
Closes: CLIBS-249