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

spike: Add TypeScriptTypes dumper #109

Closed
wants to merge 17 commits into from

Conversation

wasabigeek
Copy link
Contributor

@wasabigeek wasabigeek commented May 12, 2021

Opening a very early PR to discuss #103. I'm thinking to add a TypeScriptTypes class with a dumps method that:

Does that sound good @kiwicopple @soedirgo? I know we want this to be reusable across different languages but it seems PostgresColumn is already a decent abstraction that can go into the various language dumpers.

TODO:

  • Setup unit testing + stubbing for TypeScriptTypes
  • Test prettier for formatting
  • handle int8 column
  • handle is_nullable flags
  • handle other column types
  • handle multiple columns
  • handle grouping of columns

@kiwicopple
Copy link
Member

I will also try to write unit tests

That would be awesome. If we get to a stage where we can make the output match the output from this process, then we can slowly modify the output to resolve bugs like this.

convention for filenames / paths we'd like to follow

Not really yet, but if you just start getting something workable, we can always rename. @soedirgo - if you have some ideas/opinions on the organization of the generators, feel free to add 👍

@wasabigeek
Copy link
Contributor Author

wasabigeek commented May 19, 2021

I'm still quite far from completing but let me know if you already see any red flags @kiwicopple @soedirgo:

  • added sinon for stubbing
  • moved prettier to regular dependencies and added @types/prettier so we can use that to format the string (following the openapi-typescript library)
  • naively generated a definition string from a single column

@soedirgo
Copy link
Member

Side note: have you looked into Zapatos? Feels like it's exactly what we want, but I don't know how good the generated types are.

@wasabigeek
Copy link
Contributor Author

wasabigeek commented May 20, 2021

Side note: have you looked into Zapatos?

I have not, thanks for the heads up! Will investigate. Do you think it will be what we want longer term though, since we might want to dump to other languages as well? If we rely on PostgresColumn we'd have an abstraction that can be reused for other languages. (Though I don't quite have a vision of how we'd use this for another language)

@soedirgo
Copy link
Member

Yeah, I think it's best if different languages are handled separately. All type systems have their own idiosyncrasies, so it's best to avoid adding abstractions esp. if it might end up being inflexible for future languages.

That said, at first glance Zapatos has some type information that we don't need... (Selectable, Whereable, etc.) It serves as a good reference at least.

@kiwicopple
Copy link
Member

I do that we will need to eventually do this ourselves (to get better granularity on specialty types like GIS columns).
I wouldn't really mind having a common interface which can be extended somehow, but it's going to be a crazy amount of work then perhaps it's not worthwhile right now

@wasabigeek
Copy link
Contributor Author

wasabigeek commented May 22, 2021

@soedirgo @kiwicopple I think the core logic is done, there are still some parts to work out:

  • how do we envision this being used? For now the class takes in PostgresMeta as an arg and dumps() returns a string - is that the desired interface? Or did you expect some CLI functionality
  • how closely do we want to replicate the existing postgrest API + openapi-typescript output? For example, that adds comments for primary / foreign keys, and formats the interfaces with newlines, and also doesn't "singularize" the types

Other notes:

  • I'm not too clear on how the existing flow handles enums (I'll need to brush up on them as well), seems like zapatos has a special handling for them
  • I've opted to keep everything in one file now, potentially we could do more abstractions (e.g. a TypeScriptInterface that corresponds to a PostgresColumn) but was thinking to delay that till we have other languages in the mix
  • todo: need to handle errors in pgMeta.columns.list()

@kiwicopple
Copy link
Member

kiwicopple commented May 24, 2021

🚀

how do we envision this being used?

@soedirgo thoughts? Ultimate we should be able to do something like npx @supabase/postgres-meta generate --format typescript --output types/supabase.ts so that we can use it in the CLI too. Or something like

import { generators } from '@supabase/postgres-meta'

const outputFile = './types/supabase.ts`
const { output, error } = await generators.typescript({ file: outputFile })

how closely do we want to replicate the existing postgrest API + openapi-typescript output?

I think almost exactly for now (comments should come from Postgres Comments) and then we can improve it after we have parity

I'm not too clear on how the existing flow handles enums

Does this help?

I've opted to keep everything in one file now, potentially we could do more abstractions

Sounds good! Make it work, then refactor when we're ready

@soedirgo
Copy link
Member

Hey @wasabigeek, sorry for the late response here, got sidetracked...

I've given this some thought, and I think it's better to hold off the PR for now and wait for the updated type implementation in supabase-js 2.0 (following supabase/supabase-js#125). The reason is it's easier to start with designing the types keeping in mind it should support autocomplete & will be autogenerated through introspection, than:

  • to start with a rudimentary implementation that keeps up with the design (which will lead to a lot of breaking changes), or
  • to do both in tandem (breaking changes + more friction when designing).

Re: how it's used: not sure if it's better to implement it here or in the CLI—generating types seems to be far off the "manage Postgres objects" rhetoric, and the CLI is the only place we use the typegen for now, and introspection e.g. for the Dashboard can be done with just the postgres-meta server (and CMIIW but node-postgres, which this repo depends on, can't run in the browser).

If implemented here ideally it can be used as a library as well as standalone. npx postgres-meta is nice, but that kinda retrofits the repo as a CLI. Maybe better to expose it through the server?

Side note: not sure how we'd generate types for other languages—e.g. Dart, and most other languages, don't have an equivalent of key of which is necessary for the types in postgrest-js to work—so for now I think it's safe to assume TS-only for now, other languages will likely require metaprogramming or similar hacks.

@kiwicopple
Copy link
Member

If implemented here ideally it can be used as a library as well as standalone.

This is the main reason I would prefer it here. People can use it "outside of Supabase". Ideally this library can be useful as a standalone tool, and the CLI just becomes a thin wrapper which glues everything together.

@wasabigeek
Copy link
Contributor Author

wasabigeek commented Jun 3, 2021

Hi friends, sorry for the MIA, but looks like that was a good thing in the end 😄

It's a little hard to move forward as I'm missing quite a fair bit of context. How would you like to proceed?

FWIW, I do agree with @soedirgo that generating types is a separate concern. In my mind, type-generation could be a separate library - but I'd like to think we can leverage the abstractions here in the other library (e.g. PostgresColumn). That said I think we need to have more languages to dump before we can say it's a good abstraction. So I vote to keep it here, but ideally label it as "experimental" for now (nothing stopping anyone from using it, but we can at least say it's not protected from change).

P.S. please don't feel like we need to keep this PR, it is named "spike" after all - if it's decided this is not a good long term approach, let's close it 👍

@soedirgo soedirgo changed the base branch from develop to master September 16, 2021 11:23
@sweatybridge
Copy link
Contributor

superseded by #269

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.

None yet

4 participants