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

Stronger typings and simpler variable parsing #76

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Julusian
Copy link
Member

@Julusian Julusian commented Apr 8, 2024

This is based on some experimenting I did in the atem module bitfocus/companion-module-bmd-atem#254

I am very interested in feedback/input from others on this.

Although there is no technical reason for it, I would like to propose making using these 'strict' types mandatory to support the use of location based variables in module actions and feedbacks. This is because it will help ensure that they are handled correctly, and mean that we have control over how the parsing is done, which will help minimise any impact to modules if we need to make future changes. Either way, modules will need to make changes to support location based variables.

The aim is to provide an action and feedback definitions which are strongly typed (to benefit modules using typescipt), and at the same time try to abstract away the parsing of variables.
This has intentionally been done as a 'strict' version, without touching the existing types, so that modules can choose which version they use. This is to avoid it being a breaking change, and allow modules to migrate gradually if they want to, and to do so gradually.
Maybe in the future we will remove the looser flow, but unless it becomes incredibly messy to support both it likely won't be worth the effort.

As an example, some simple actions which use these types: https://github.com/bitfocus/companion-module-bmd-atem/blob/main/src/actions/aux.ts
And example feedbacks: https://github.com/bitfocus/companion-module-bmd-atem/blob/main/src/feedback/aux.ts
And presets: https://github.com/bitfocus/companion-module-bmd-atem/blob/main/src/presets/aux.ts

TODO:

  • Some further thought is needed on upgrade scripts, as they will need to be aware of how the options are stored.
  • StrictOptionsImpl should cache the values it has fetched/validated, so that modules don't need to think about whether they might be parsing the same string multiple times.
  • It would be a good idea to take the opportunity to review the input field types, and consider if they need any tidying.
  • The types of StrictOptions won't play nicely if we want to allow any support useVariables: true on number input fields. Perhaps all of the getters should be async so that we can allow this?
  • Import and update the implementation of translating the existing flow to support these new types.
  • Perhaps there should be another layer of interfaces, so that the StrictOptions knows what input field type is being used, and can reconfigure itself appropriately? Or perhaps StrictOptions should be a little less strict and shouldn't try to validate return types?

@dnmeid
Copy link
Member

dnmeid commented Apr 9, 2024

When I started with the AWJ module I also wanted to have better typings but I'm still quite a ts noob.
I had set up a little mock up by that time: TS Playground
I never started to apply this to Companion, but maybe it is now somehow useful.
The basic idea is to infer the types from the implementation.
When I abandoned it, there had been two challenges:

  • Actually I wanted to type the object, so you can see any errors in the object itself and not only when using the object
  • I couldn't find a way to convince ts that whenever I use e.g. 'id', this should be treated as a const without having to declare it as const.

Regarding separate strict types: why is it needed at all? If we just do a stronger typing, the new types should extend the old ones. That means there should not be a change for any module. If a module fails with stronger typings than it means that there is an error.

@Julusian
Copy link
Member Author

Julusian commented Apr 9, 2024

The basic idea is to infer the types from the implementation.

Ooh, that is using some typescript stuff I wasn't aware was possible.
It has a similar aim, I suppose there is a discussion about whether we want to infer types or expect them to be declared explicitly.

Regarding separate strict types: why is it needed at all? If we just do a stronger typing, the new types should extend the old ones. That means there should not be a change for any module. If a module fails with stronger typings than it means that there is an error.

One of my aims here was to replace the options object with a class, so that parsing of variables/expressions can be handled easier, as well as validating types. That change alone makes the method signatures incompatible with the existing ones.
But yes, even though I have called these 'strict', they will work just fine if they are told the options type is any (or might need to be Record<string, any>), so will be perfectly usable in a loose mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants