Repository navigation
Add controller messaging system - #377
Conversation
0e63938 to
e803cc0
Compare
c86d06e to
159d49e
Compare
159d49e to
756e986
Compare
There was a problem hiding this comment.
I wasn't able to get rid of this any 🤔 Using unknown or unknown[] seemed to upset TypeScript. What I wanted here was to express that I don't know anything about these parameters, but I don't know how to do that.
Though, maybe it's fine to leave this as-is. There are unit tests for zero parameters, one parameter, and multiple parameters (of various different types). Those should be sufficient for ensuring we're not assuming anything about these parameters in this class.
There was a problem hiding this comment.
When i was trying to figure out types I had to do some 'any' to allow it to work, even though I had full type safety in practice. Hard to explain/understand why that was true but I think it's fine as is
|
Attenuation is added in this follow-up PR: #378. It's still a work-in-progress, but that should give some idea what attenuation could look like. |
|
I don't yet have tests for ensuring this is type-safe. There is no straightforward way of testing this, as any misuse of types would fail to compile, and JavaScript tests wouldn't test the types at all. There are some libraries that could help here though. I've opened #379 for investigating this further. In the meantime I've been testing that this is type-safe by manually editing the unit tests into various type violations, and ensuring I get an error when I run the tests. |
brad-decker
left a comment
brad-decker
left a comment
There was a problem hiding this comment.
LGTM, the removal of base controller subscribe/unsubscribe can happen here or in a follow up. Happy to see this taking shape, and can't wait to use it in the extension :)
756e986 to
5202621
Compare
|
Thanks for the review @brad-decker ! Unfortunately I've just discovered that I forgot to document all of these methods though 🤦. So I'm gonna push one more commit in a few minutes to address that shortcoming. |
6dec3da to
34f5f13
Compare
|
Alright, all methods should now be documented! This is ready for review again. |
|
It'd be nice to get more feedback on this one before merging since it'll have a large impact upon future controller work, so I've tagged more people for review. No pressure though. I'll leave this open another couple of days, or until it gets another approval or two. |
c4fb8f4 to
c267a31
Compare
Adds a controller messaging system, which will be used to facilitate inter-controller communication. The controller messenger acts as a message broker, passing actions and events back and forth between controllers. It is _fully type safe_. This idea was described in the Controller Messaging System proposal[1]. This controller messenger doesn't yet support attenuation, so it's not yet possible to restrict which actions and events are interacted with. That will be coming in a later PR. [1]: https://www.notion.so/Controller-Messaging-System-617efb02b9e54bd0a0b0e44c6f776d85
The base controller no longer exposes methods to subscribe and unsubscribe to state changes. Instead all subscriptions and unsubscriptions are done with the controller messaging system.
This event name matches the naming convention of the event prefix, which is also camelCase.
JSDoc comments have been added for each method on the controller messenger instance. Additionally, the `event` parameter has been renamed to `eventType`, and the `action` parameter has been renamed to `actionType`. This was done while adding docs because it made the documentation easier to understand.
These types were only used internally. We can export them again if and when that becomes useful.
c267a31 to
1671284
Compare
2 participants
Adds a controller messaging system, which will be used to facilitate inter-controller communication. The controller messenger acts as a message broker, passing actions and events back and forth between controllers. It is fully type safe.
This idea was described in the Controller Messaging System proposal.
This controller messenger doesn't yet support attenuation, so it's not yet possible to restrict which actions and events are interacted with. That will be coming in a later PR.