Convert frontend-core API client to Typescript #15096
Conversation
… only watch src dirs
|
QA Wolf here! As you write new code it's important that your test coverage is keeping up. |
…ormity of flattened function params
…o frontend-core-ts-2
| if (selectedRows?.length) { | ||
| payload.rows = selectedRows.map(row => row._id) | ||
| } | ||
| if (sorting) { | ||
| payload.search.sort = sorting.sortColumn | ||
| payload.search.sortOrder = sorting.sortOrder | ||
| payload.sort = sorting.sortColumn |
There was a problem hiding this comment.
❤️
I guess this was broken? Or were we supporting both formats?
There was a problem hiding this comment.
It wasn't broken, but I've just flattened the structure a little to make it simpler. We used to do this logic inside the API client code, but there was no point in wrapping things in a search object just to pull them out again later. So the result sent to the API is identical but it's a bit simpler to use it now :)
| export function createPluginsStore() { | ||
| const { subscribe, set, update } = writable<Plugin[]>([]) | ||
|
|
||
| async function load() { | ||
| const plugins = await API.getPlugins() | ||
| const plugins = (await API.getPlugins()) as Plugin[] |
There was a problem hiding this comment.
Could the API not be typed, instead?
There was a problem hiding this comment.
The API actually is typed and returns a FetchPluginResponse, which is typed as
export type FetchPluginResponse = Plugin[]so I'm only casting to Plugin[] because I'd prefer to store that type, even though there is no difference. It just feels wrong to store FetchPluginResonse in the store when it's actually just an array of plugins. I'm happy remove this if you'd prefer - I'd actually appreciate knowing what you think is better because I knew this casting was redundant but it just seemed "nicer".
There was a problem hiding this comment.
I think my preference is to do something like const plugins: Plugin[] = await API.getPlugins() in these type of scenarios, this isn't casting in this case as Typescript will infer whether the types overlap enough for it to work.
This is similar to using a base class in OO, e.g. a Pet and an extended class Dog - you can do something like const pet: Pet = new Dog(), however in TS this is a lot more powerful since it looks into the structure of the two types rather than using inheritance.
Using as Plugin[] is more dangerous, because you are forcing the conversion - there is some protections still if the two types don't overlap at all/it makes no sense, but you'll lose protections around things like undefined checks.
There was a problem hiding this comment.
Sounds good! I'll go for that approach instead of the as xxx. Thank you 🙏
| "../pro/src", | ||
| "../types/src", | ||
| "../shared-core/src", | ||
| "../string-templates/src" |
| } | ||
| } catch (error) { | ||
| delete cache[url] | ||
| return null | ||
| throw `Failed to parse response: ${error}` |
There was a problem hiding this comment.
Do you know the reason why we were not throwing, before?
There was a problem hiding this comment.
I think it was probably just a mistake from before, but I don't honestly know... We need to throw now with TS, since we can't return the proper type when we hit this - but yea I think we probably should have been always throwing, for correctness.
| suppressErrors: boolean | ||
| } | ||
|
|
||
| export type APIClient = BaseAPIClient & |
There was a problem hiding this comment.
Hectic, but good typing! :)
|
Thanks for the review @adrinr! This was a monster 😅 |
| name: detail.restoreBackupName, | ||
| }) | ||
| await fetchBackups(filterOpt, page) | ||
| } else if (detail.type === "backupUpdate") { |
There was a problem hiding this comment.
Is the backup update no longer required?
There was a problem hiding this comment.
Nope I believe we actually stopped using and allowing custom naming of backups a while ago, so this code existed but was never used. So just pruning dead code with this.
| @@ -35,7 +35,24 @@ export function createUsersStore() { | |||
| } | |||
|
|
|||
| async function invite(payload) { | |||
| return API.inviteUsers(payload) | |||
| const users = payload.map(user => { | |||
There was a problem hiding this comment.
I assume all this extra work is to make sure that the user fits the API typing?
There was a problem hiding this comment.
Yep I believe so! This code is basically just cut and pasted from inside the API client, but I'd rather expose it here and have a proper typed structure as the payload for the inviteUsers call, rather than what we used to do which is pass in an arbitrary frontend structure which then gets transformed into the proper typed structure inside the API client code before being sent.
This just moves all the frontend logic here and keeps the param clean and typed 👌
| * @param tableId the ID of the table | ||
| * @param rowActionId the ID of the row action to update | ||
| */ | ||
| update: async ({ tableId, rowActionId, name }) => { |
There was a problem hiding this comment.
Was the update no longer needed?
There was a problem hiding this comment.
Yep this was also dead code! Row actions are now renamed by renaming the automation, using the normal automation endpoint, rather than via this one.
…o frontend-core-ts-2
Description
This PR contains the full conversion of the frontend-core API client to typescript.
Along with the TS conversion, certain endpoints have had their arguments flattened and the calling code updated accordingly.
My usage of
typevsinterfaceis pretty much arbitrary so I welcome any recommendations or changes to that, as well as the location of types.All new typescript code is contained in
frontend-core/src/api. Every other change is just to accommodate function signature changes, which are a combination of flattening params and rearranging things slightly to suit types better.