Generate TS interfaces from OAS #3
Conversation
Rather than redeclaring all of our types, let's generate them from our OAS JSON file and save some repetition.
I also added notes for a few things we should have stories for.
React needs to be v16.8 or higher for hooks, and React v17 should work just fine
| @@ -46,6 +46,8 @@ yarn test | |||
| yarn storybook | |||
| # Build storybook once | |||
| yarn build-storybook | |||
| # Fetch our openapi spec and generate fresh TypeScript types | |||
| yarn generate | |||
There was a problem hiding this comment.
nit: i changed this in octane-portal to be yarn codegen as i felt it was more self-descriptive. Mentioning it for consistency.
| }, | ||
| "peerDependencies": { | ||
| "react": "^16.0.0" | ||
| "react": ">=16.8.0" |
| responses: { | ||
| /** Default error response */ | ||
| DEFAULT_ERROR: { | ||
| content: { | ||
| 'application/json': components['schemas']['Error']; | ||
| }; | ||
| }; | ||
| /** Unprocessable Entity */ | ||
| UNPROCESSABLE_ENTITY: { | ||
| content: { | ||
| 'application/json': components['schemas']['Error']; | ||
| }; | ||
| }; | ||
| }; | ||
| } |
There was a problem hiding this comment.
great that we have more visibility into the generated error types. This was something i felt was missing previously.
| description?: string | null; | ||
| /** Whether measurement values are to be considered incremental (versus a running total) */ | ||
| is_incremental?: boolean; | ||
| meter_type?: unknown; |
There was a problem hiding this comment.
came here to look at what the types were for DB enums. looks like unknown. Any thoughts on how to approach this? For some well-defined types like meter_type, I feel it's reasonable to manually represent that, knowing it's unlikely to change in the future.
There was a problem hiding this comment.
for some of these, we may need to write a script that uses the openapi-typescript API. they have a few examples for how to deal with custom field types:
https://github.com/drwpow/openapi-typescript#custom-formatter
there are some properties, like meter_name on MeteredComponents, that are unknown because the type property is missing from the actual openapi.json file. I don't totally know what to do about that but I talked with Ahmed for a bit yesterday to see if there were options (we didn't find anything obvious).
There was a problem hiding this comment.
Yeah we were doing similar for the graph QL codegen step as well. It's good to get rid of the unknown but also exposes us to some potential bugs if we aren't careful
| * Test cases to implement: | ||
| * 1. No base price | ||
| * 2. Multiple metered components | ||
| * 3. Discounts | ||
| * 4. Limits | ||
| */ |
There was a problem hiding this comment.
nit: these cases seem most applicable to the individual price plan (rather than the plan picker). Seems like two separate things that we'd want to distinctly and individually test/validate.
| limit: 400.0, | ||
| meter_name: 'storage', | ||
| price_scheme: { | ||
| display_name: null, | ||
| name: null, | ||
| prices: [{ price: 300.0 }], | ||
| scheme_type: 'PriceSchemeType.FLAT', | ||
| time_unit_name: null, | ||
| unit_name: 'gigabyte', | ||
| }, |
There was a problem hiding this comment.
if you're using storage which i presume is a gauge, I think you'd want to include an associated time unit name, otherwise it's unclear what unit is being billed on (i.e. if my price is 300 per GB/month then 1 GB at the halfway point of the month should total $150)
There was a problem hiding this comment.
I made a storage meter and billed on it in a few different ways. all this data came straight from the API; it's not hand-written, to be clear.
There was a problem hiding this comment.
It's odd that you could create a scheme on a gauge without having to specify a time unit name, though maybe that was just a validation step added as part of the portal UI
There was a problem hiding this comment.
oh wow the answer is even dumber than that; Storage is a counter.
There was a problem hiding this comment.
haha that makes a lot of sense, glad nothing is broken :P
| const billingSchedule = | ||
| period === 'month' ? 'Billed monthly' : `Billed every ${period}`; | ||
| const formattedBasePrice = formatCurrency(basePrice); | ||
| const hasBasePrice = basePrice != null; |
There was a problem hiding this comment.
should we add similar utils for checking for null/undefined as in oc portal? I typically liked those for readability, though the naming i believe had some overlap with some other system utils (just recall having some autoimport issues a few times)
There was a problem hiding this comment.
yeah, I was thinking about that. I learned the hard way (when implementing Limits) that if the lhs resolves to 0 (instead of null or undefined), a literal 0 will get rendered.
| {hasBasePrice && ( | ||
| <div className='base-price'> | ||
| <span className='label starting-at'>Starting at </span> | ||
| <span className='cost'>{formattedBasePrice}</span> |
There was a problem hiding this comment.
nit: use a more representative className, i.e. className='base-price-value'
Mainly want to avoid the use of the cost nomenclature and think about it more as a price.
There was a problem hiding this comment.
ugh i accidentally pushed that commit into the next PR's branch but it's there.
Rather than redeclaring all of our types, let's generate them from our OAS JSON file and save some repetition.
Some specifics:
yarn generatenow generatessrc/apiTypes.tsin a way that agrees with out prettierrc.PricePlanandMeteredComponenttypes in the PlanPicker component and story.