-
Notifications
You must be signed in to change notification settings - Fork 38.8k
refactor: Upgrade to TypeScript 5.9.2 #17955
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
base: master
Are you sure you want to change the base?
Conversation
@@ -266,7 +266,7 @@ export class ChatService { | |||
private stringifyRawData(data: RawData) { | |||
const buffer = Array.isArray(data) | |||
? Buffer.concat(data.map((chunk) => Buffer.from(chunk))) | |||
: Buffer.from(data); | |||
: Buffer.from(data instanceof ArrayBuffer ? new Uint8Array(data) : data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ArrayBuffer
is no longer a supertype of TypedArray
types like Buffer
and Uint8Array
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cubic analysis
1 issue found across 4 files • Review in cubic
React with 👍 or 👎 to teach cubic. You can also tag @cubic-dev-ai
to give feedback, ask questions, or re-run the review.
const buffer = Array.isArray(data) ? Buffer.concat(data) : Buffer.from(data); | ||
const buffer = Array.isArray(data) | ||
? Buffer.concat(data) | ||
: Buffer.from(data instanceof ArrayBuffer ? new Uint8Array(data) : data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary creation of Uint8Array
causes an extra memory copy; Buffer.from(ArrayBuffer)
already works and avoids the overhead. (Based on your team's feedback about focusing on performance-impacting issues.)
Prompt for AI agents
Address the following comment on packages/cli/src/task-runners/task-broker/task-broker-ws-server.ts at line 104:
<comment>Unnecessary creation of `Uint8Array` causes an extra memory copy; `Buffer.from(ArrayBuffer)` already works and avoids the overhead. (Based on your team's feedback about focusing on performance-impacting issues.)</comment>
<file context>
@@ -99,7 +99,9 @@ export class TaskBrokerWsServer {
const onMessage = async (data: WebSocket.RawData) => {
try {
- const buffer = Array.isArray(data) ? Buffer.concat(data) : Buffer.from(data);
+ const buffer = Array.isArray(data)
+ ? Buffer.concat(data)
+ : Buffer.from(data instanceof ArrayBuffer ? new Uint8Array(data) : data);
const message: RunnerMessage.ToBroker.All = JSON.parse(
</file context>
const buffer = Array.isArray(data) ? Buffer.concat(data) : Buffer.from(data); | ||
const buffer = Array.isArray(data) | ||
? Buffer.concat(data) | ||
: Buffer.from(data instanceof ArrayBuffer ? new Uint8Array(data) : data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cubic has a point here. When we wrap data
in an UInt8Array
it causes an unnecessary memory copy (see this). I think it's because the Buffer.from
overload fails to infer the correct usage. This works and doesn't do extra allocations (tho not the prettiest):
const buffer = Array.isArray(data)
? Buffer.concat(data)
: data instanceof ArrayBuffer
? Buffer.from(data)
: data;
It also seems that we have done the memory copy all this time without realizing (as Buffer.from
copies when the input is another instanceof Buffer
). We could fix this now or merge this first and I can fix it afterwards.
https://devblogs.microsoft.com/typescript/announcing-typescript-5-9/