Skip to content
This repository was archived by the owner on Mar 5, 2024. It is now read-only.

Conversation

@GoNZooo
Copy link

@GoNZooo GoNZooo commented Jan 7, 2021

This narrows down the RPC types to what the comment specifies, effectively making it obsolete and actually enforced.

Literal values are their own types, which means we can specify these unions of literals just fine.

The Callback signature is also very naïve. any is not a useful type, but rather just a way to turn off the type system when there is a lack of understanding of what's going on. A callback taking JSON data of an unknown shape should instead be specified as taking unknown, which means the callback itself will have to ascertain what the shape of the data is, via type guards.

unknown is effectively a version of any that cannot be passed to something requiring a more specific type, which means it's a type-safe way to refer to data that is of unknown shape.

Further work on the RPC functions should probably be done, with a union of possible commands being created (which would also then remove the need for the stringly typed data parameter). In general, the code, small as it is, is full of places where thinking about the possible values of things and actually nailing these down could be useful.

It's clear, for example, that there is a valid set of inputs and a shape to jmsg in the code, except the expectation is never validated or otherwise established. JSON.parse is a nebulous function to use for these things, as it returns any. This illustrates the issue with letting any into the codebase. If it returned unknown, the code would instead have to actually establish that it's dealing with the correct data.

any is also used for MINIMA_PARAMS in place of a more precise type. I believe this is likely the reason for the nonsensical return MINIMA_PARAMS[paramname] in a function that returns void (form.params, minima.ts:555).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant