search

rilldata / rill

Rill is a tool for effortlessly transforming data sets into powerful, opinionated dashboards using SQL. BI-as-code.
https://www.rilldata.com
Apache License 2.0
1.7k stars 114 forks source link

Make sure API types are correctly passed through to typescript type definitions. #3735

Open bcolloran opened 10 months ago

bcolloran commented 10 months ago

supports work on https://github.com/rilldata/rill/issues/3373, and generally making the client code more robust and reliable.

Problem: currently every field in every type produced by our code gen pipeline is marked as optional. This can cause (a) incorrect casts, if we assume a field that is marked optional must be present (b) a proliferation of noisy, unnecessary, and error-prone guard code in the client if we take at face value that every field really is optional.

@begelundmuller and I have discussed this, and I think we have both short and long term plans to address the problem.

(1) In the short term, @begelundmuller has said that the runtime team team can take on the task of providing field-level comments that will be passed through to the TS types, so that each field can be manually marked as OPTIONAL or REQUIRED in the TS types, thereby allowing the applications team to at least go look up the types and confidently cast REQUIRED fields without error.

(2) There may be technical solutions that will allow us to improve our codegen types without needing to mark every field as optional. This may include moving to connectRPC or some other solution.

@begelundmuller and I discussed this late November, and hoping we schedule part (1) as chore for the runtime team. From the outside, it doesn't look like the kind of thing that should be a huge amount of work -- probably something that we just need to grind out with a couple hours of work. But I'm pretty far from it, and if it does seem like a lot of work, I'd love to see us put in place a strategy for dividing that up among the runtime team so that we're making some incremental progress every week. This will be of tremendous value to the work I'm doing in #3373, and will help a lot in terms of reducing data-flow uncertainty in the client

(cc @ericpgreen2 @nishantmonu51)

bcolloran commented 10 months ago

I started looking at the improving the null checking in web-admin, and it will be especially critical to get the API types to correctly represent whether fields are required or not to tighten the null checking for that chunk of the repo, because so much of that code is really a thin wrapper around the runtime so it's interacting with the API a lot more directly than many other parts of the front-end, and it's really important to be able to make accurate assumptions about which fields are present to avoid a ton of noisy undefined checks or risky type casts. I consider further progress on #3373 blocked until we can get these API types in place.

(fyl @ericpgreen2 since web-admin was potentially something you were going to look into -- i started trying to tackle it just to lend a hand, but after going through a few files, I think we should wait until we can get better type info from the runtime)

bcolloran commented 9 months ago

@begelundmuller @nishantmonu51 bump on this -- I'd love to see work started on the short-term and lower-effort approach that @begelundmuller proposed in November. From the discussion we had at that time, it sounded like that would be a chore of a few hours to go through and add comments that can be passed through to the TS types.