Unify performanceIsometric implementation, simplify typing, and enable sha.js typing in client-utils and common-utils #18253
Conversation
…e dependency from odsp-driver
github-actions
bot
added
area: driver
| @@ -132,7 +132,7 @@ export class DocumentDeltaConnection | |||
| logger.sendErrorEvent( | |||
| { | |||
| eventName: "DeltaConnection:EventException", | |||
| name, | |||
| name: name as string, | |||
There was a problem hiding this comment.
EventEmitterEventType explicitly permits name to be string | symbol, but sendErrorEvent definitely wants a string.
Probably the best option is to restrict EventEmitterEventType to just string for simplicity, but I don't do that in this change as it would be a public API change.
There was a problem hiding this comment.
Not sure what this is accomplishing. At runtime if this was a Symbol it'll still be one, and I don't understand how the cast would help TS at development time.
There was a problem hiding this comment.
The typing changes suddenly made TS realize this should fail type checking, so the cast allows it to build again.
If a Symbol were being passed it would throw later down the line (e.g. in something like convertToBasePropertyTypeUntagged) since there's not really a reasonable way to convert a Symbol to something that can be serialized/logged. So this cast just makes that assumption explicit; that although name technically could be a Symbol here, we're really assuming it's not if we're willing to pass it into sendErrorEvent.
My comment above is basically saying "why risk it, it would make more sense to just ban Symbol as eventName throughout the repo and force everyone to use strings, no one like Symbols anyway". Then this cast could go away. But doing so would technically be a breaking change.
⯅ @fluid-example/bundle-size-tests: +6.87 KB
Baseline commit: 3392bac |
| "paths": { | ||
| "perf_hooks": ["types/perf_hooks.d.ts"], | ||
| }, | ||
| "types": ["events"], |
There was a problem hiding this comment.
Why isn't this needed? Is this what's causing the node types to be referenced in the api docs?
There was a problem hiding this comment.
I think the node types being referenced in the api docs is just because we're finally actually using sha.js types (which themselves reference node types). The api report changed not from the tsconfig change (first commit in this PR), but when I actually started using the typed objects (second commit in this PR). I can revert the "events" change here if you like (doesn't seem to make a difference really), but it doesn't change the api docs.
There was a problem hiding this comment.
Confirmed with a messy hack that the node types do indeed source from the usage of @types/sha.js, specifically used to avoid explicitly typing Hash (guessing this was just a shortcut).
Since Hash doesn't actually show up on our public API surface, seems like api-extractor shouldn't strictly need to include the reference? Though I don't know much about api-extractor internals.
| @@ -4,6 +4,8 @@ | |||
|
|
|||
| ```ts | |||
|
|
|||
| /// <reference types="node" /> | |||
| @@ -11,3 +11,8 @@ | |||
| */ | |||
| export type IsomorphicPerformance = Partial<Performance> & | |||
| Pick<Performance, "clearMarks" | "mark" | "measure" | "now">; | |||
|
|
|||
| /** | |||
| * @internal | |||
There was a problem hiding this comment.
Can we add some docs while we're here?
There was a problem hiding this comment.
Added in latest!
| @@ -132,7 +132,7 @@ export class DocumentDeltaConnection | |||
| logger.sendErrorEvent( | |||
| { | |||
| eventName: "DeltaConnection:EventException", | |||
| name, | |||
| name: name as string, | |||
There was a problem hiding this comment.
Not sure what this is accomplishing. At runtime if this was a Symbol it'll still be one, and I don't understand how the cast would help TS at development time.
| @@ -74,7 +74,7 @@ export class DocumentDeltaConnection | |||
|
|
|||
| private _details: IConnected | undefined; | |||
|
|
|||
| private trackLatencyTimeout: number | undefined; | |||
| private trackLatencyTimeout: ReturnType<typeof setTimeout> | undefined; | |||
There was a problem hiding this comment.
Huh, how was this passing type checks before when we were assigning to it?
There was a problem hiding this comment.
Not sure - same as the change below, the typing changes tickled TS in some way that made it realize there was bad typing in this file. Kind of spooky that they weren't caught earlier, but after investigating I think both were correctly identified as errors.
Last year I fixed performanceBrowser to use globalThis to make it work in worker threads (#12081), but I really should have done the same thing in Node and unified the two. This change completes that unification. As a result, we can also eliminate some atypical tsconfig settings. Did this for both client-utils and common-utils even though it's deprecated in common-utils, because why not :)
Secondly, updated sha.js types to latest (and removed the unused dep from odsp-driver). Converted usage of sha.js to not use internal modules, which was defeating strong typing there.
In the process, all of this uncovered some typing errors and quirks that are also adjusted for in this change.