Migrate sync package to TypeScript #71099
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
manzoorwanijk
left a comment
There was a problem hiding this comment.
This looks great. I just have some suggestions for further improvements.
| * @type {Record<string,Record<string,()=>void>>} | ||
| */ | ||
| const listeners = {}; | ||
| const listeners: Record< string, Record< string, () => void > > = {}; |
There was a problem hiding this comment.
We can use the handy type alias here
| const listeners: Record< string, Record< string, () => void > > = {}; | |
| const listeners: Record< string, Record< string, VoidFunction > > = {}; |
| const signalingConn = map.setIfUndefined( | ||
| signalingConns, | ||
| url, | ||
| // Only this conditional logic to create a normal websocket connection or | ||
| // an http signaling connection was added to the constructor when compared | ||
| // with the base class. | ||
| url.startsWith( 'ws://' ) || url.startsWith( 'wss://' ) | ||
| ( url.startsWith( 'ws://' ) || url.startsWith( 'wss://' ) | ||
| ? () => new SignalingConn( url ) | ||
| : () => new HttpSignalingConn( url ) | ||
| : () => | ||
| new HttpSignalingConn( | ||
| url | ||
| ) ) as () => SignalingConn |
There was a problem hiding this comment.
We can get rid of the assertion at the end like this
map.setIfUndefined<
SignalingConn | HttpSignalingConn,
string,
Map< string, SignalingConn | HttpSignalingConn >
>(...)| buffer.fromBase64( m.data ), | ||
| room.key | ||
| ) | ||
| // @ts-ignore |
There was a problem hiding this comment.
Can we please instead use // @ts-expect-error with a comment explaining why?
Also, if it's about the argument, then I would prefer moving the comment inside the parentheses to apply it appropriately.
// @ts-expect-error
.then( execMessage );vs
.then(
// @ts-expect-error
execMessage
);Former ignores a wide ranges of TS errors. For example, if then is not a function, it won't error, which is what we probably don't want.
What?
Part of: #67691
Migrating the sync package to Typescript.
Why?
Type safety.