Convert the dispatcher to TypeScript #4593
Conversation
Due to TypeScript and flux's types being annoying and highly typesafe, we need an AsyncActionPayload which intentionally doesn't use the 'action' property. This looks a bit awkward, though for the rare cases we do actually fire async actions it should be fine enough. The call signature changes slightly for async events, therefore this commit also updates its usages for async events. The `fire()` function is to be used in a future commit. Remove biased comment
We're expecting to have a whole bunch of types for the dispatched payloads, so pull the thing into a directory we can throw them in.
The definitions take up a lot of space which makes it hard to see the dispatcher class, so break them out.
This is a relatively obvious dispatch action that doesn't require a lot of complicated type definitions, so should be a good candidate to prove the thing works. If for some reason the thing stops working, we've done something wrong.
This also adds a bit of generic types to the dispatch call so we don't confuse the tsx parser by using `dis.dispatch(<ViewUserPayload>{...})` as it thinks that's supposed to be a component. We still get type safety, and the thing remains happy with the generics approach.
Like a5f3318, this proves that the new dispatcher conversion works for fire-and-forget style dispatches too. This is another obvious-if-broken and generally safe conversion to make. Other actions which can be dispatched this way have been excluded for reasons mentioned in the Action enum's comments.
| // focus-visible is a Polyfill for the :focus-visible CSS pseudo-attribute used by _AccessibleButton.scss | ||
| import 'focus-visible'; | ||
| // what-input helps improve keyboard accessibility | ||
| import 'what-input'; | ||
|
|
||
| import Analytics from "../../Analytics"; | ||
| import { DecryptionFailureTracker } from "../../DecryptionFailureTracker"; | ||
| import {MatrixClientPeg} from "../../MatrixClientPeg"; | ||
| import { MatrixClientPeg } from "../../MatrixClientPeg"; |
There was a problem hiding this comment.
Got a bunch of unused imports here
There was a problem hiding this comment.
yea, though it's best to leave the cleanup for a future PR (just to avoid having a single PR doing too much).
There was a problem hiding this comment.
That being said, apparently my IDE had some thoughts about how to deal with this class - will fix.
There was a problem hiding this comment.
Actually, it looks like the imports should already be cleaned up - which imports are unused?
There was a problem hiding this comment.
On second look it looks fine
| * @see asyncAction | ||
| */ | ||
| public static fetchJoinedGroups(matrixClient: MatrixClient): AsyncActionPayload { | ||
| return asyncAction('GroupActions.fetchJoinedGroups', () => matrixClient.getJoinedGroups(), null); |
There was a problem hiding this comment.
Can we enum these ids?
There was a problem hiding this comment.
We can, though it raises a question of where to stop enuming things. There's a few hundred references to .dispatch(), which could lead to nearly a hundred different enums we need to define. This is why it's recommended as something to lazily do
There was a problem hiding this comment.
Seems fine without in that case
| * @see asyncAction | ||
| */ | ||
| public static tagRoom( | ||
| matrixClient: MatrixClient, room: Room, |
There was a problem hiding this comment.
Is there a way to avoid typing the args twice? Here it's once in the jsdoc and in the ts formal args. Could we drop types in the jsdoc, or find a tool which can annotated any generated jsdoc with the correct types?
There was a problem hiding this comment.
The jsdoc isn't really valid without a type, and wee don't actually parse the jsdoc into anything. Modern IDEs tend to look at the actual function definition rather than the jsdoc, though the jsdoc does fill in some holes. Other IDEs prefer the jsdoc and treat it as more accurate than the function definition for some reason :(
It's only really an issue when refactoring - modern IDEs also infer types from the definition when starting the jsdoc comment block.
There was a problem hiding this comment.
Just wish there was a way to avoid them getting out of sync.
There was a problem hiding this comment.
yea :(
It's a largely unsolved problem as far as I'm aware. We could run the jsdoc into actual documentation which would result in it typechecking the signature, but if we don't plan on publishing those docs it feels a bit too high of overhead.
This does a number of things (sorry): * Estimates the type changes needed to the dispatcher (later to be replaced by #4593) * Sets up the stack for a whole new room list store, and later components for usage. * Create a proxy class to ensure the app still functions as expected when the various stores are enabled/disabled * Demonstrates a possible structure for algorithms
For #4253 and similar PRs.
Requires element-hq/element-web#13666
Note: this is specifically requesting review from @JorikSchellekens for the experience of code review. Others are welcome to review as always, however.
The commits here should describe what is going on, though in general: the
dispatch()function definition has changed slightly to be more typesafe, and to appease TypeScript's demands about argument types. In order to maintain thesyncparameter in the signature, we need to define a default that doesn't conflict with the super implementation. This is also whyActionPayloadcan't be a function and instead we hack anAsyncActionPayloadinto the mix - it doesn't likeActionPayload | functionas a type surface.The
fire()function is new with this PR, and should lead to faster shorthand dispatches where needed. It is intended for fire-and-forget actions, usually stuff that opens a dialog or causes a state machine state change. The more complicated stuff (meaning anything that needs additional context/parameters) still go throughdispatch().Not all actions are converted here for sake of the reader. It would be a massive diff and a bit sketchy. Instead, a couple strategic obvious-when-broken actions have been converted to prove the implementation works and to demonstrate what it looks like to convert an action. Ideally as a team we lazily do this conversion over time to avoid breaking things, as a bad conversion in some of these actions could lead to unnoticed errors and problems.