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

The future of runtime type assertions in node-groupme #57

Open
not-so-smart opened this issue Oct 20, 2021 · 2 comments
Open

The future of runtime type assertions in node-groupme #57

not-so-smart opened this issue Oct 20, 2021 · 2 comments
Labels
help wanted Extra attention is needed meta Housekeeping tasks and repository stuff

Comments

@not-so-smart
Copy link
Member

Type assertions are great for making sure that our (homemade) types are accurate and fully in sync with what the API sends and expects from us. However, there's a case to be made against validating json structures during runtime.

I would appreciate any feedback/suggestions as to whether we should continue using type checkers since it is a major design decision both for node-groupme and for the groupme-api-types repository.

Pros

  • Ensures that runtime data fulfills compile-time assumptions made on that data (i.e. you can be confident when intellisense says a field/property will exist in your code)
  • Makes debugging easier by pinpointing type errors to their root source (the api response) rather than letting something slip through with an unexpected type and causing unexplainable problems later
  • Most importantly, helps to improve our type definitions by throwing errors when they are incorrect.

For example, the type of APIGroup#join_question is currently set to null because my sample set (the groups I was in at the time) didn't have any join questions when I wrote those types. As a result, node-groupme currently throws an error upon trying to fetch groups with a join question set:

-----
API request
url: https://api.groupme.com/v3/groups
-----
(node:18316) Error: Invalid value {"type":"join_reason/questions/text","text":"are you a bot?"} for type null

This is valuable because it lets us immediately know that the types are wrong and fix them. However, it is worth considering whether this is worth the costs of type checking every single request.

Cons

  • Lots of overhead at runtime to validate objects that are valid 99% of the time; if our types are always correct then checking them is completely redundant
  • Workflow/logistical complications -- generating transformers is hard, keeping transformers up to date with types is even harder
  • discord.js doesn't do it https://github.com/discordjs/discord-api-types

Currently I am leaning toward discontinuing the transformers as we transition to groupme-api-types. Any thoughts or opinions on this one way or the other are appreciated.

@not-so-smart not-so-smart added help wanted Extra attention is needed meta Housekeeping tasks and repository stuff labels Oct 20, 2021
@not-so-smart not-so-smart pinned this issue Oct 20, 2021
@2CATteam
Copy link

Why not make a debug mode? Add an argument to the constructor, and then if that argument is true, then and only then check typing

@not-so-smart
Copy link
Member Author

debug mode or a type checking flag would be the best of both worlds, but it still requires generating transformers, which I'm still not entirely sure how to do

the ones in /interfaces were generated by quicktype and if we were to actively maintain transformers there would need to be a legitimate workflow in place for building transformers automatically as types are updated

I haven't looked too much into transformers, but if anyone is particularly attached to them and knows of a library that can easily be integrated into the types package for this purpose, I'd be more than happy to take suggestions and see if we can work something out

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed meta Housekeeping tasks and repository stuff
Projects
None yet
Development

No branches or pull requests

2 participants