-
Notifications
You must be signed in to change notification settings - Fork 13
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
fix(typegraph): send rpc message in chunks in the TS typegraph client #904
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #904 +/- ##
==========================================
- Coverage 77.46% 77.46% -0.01%
==========================================
Files 149 149
Lines 18244 18263 +19
Branches 1785 1785
==========================================
+ Hits 14133 14147 +14
- Misses 4088 4093 +5
Partials 23 23 ☔ View full report in Codecov by Sentry. |
// split into 32-KiB chunks | ||
const chunkSize = 32758; // 32 KiB - 10 bytes for "jsonrpc^: " or "jsonrpc$: " | ||
for (let i = 0; i < message.length; i += chunkSize) { | ||
const chunk = message.slice(i, i + chunkSize); |
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.
Since the message length returns the number of code units not the characters. With string slice there is still a risk of cutting in the middle of a character.
I suggest using Array.from(message)
to get an actual iterator to the characters then perform slice on that. That would also mean additional fixes around the actual total length though.. but if it's about sending raw bytes it should be fine I guess.
const text = "Hello👍";
console.log(text.length); // 7 not 6
console.log(text.slice(0, 6));
console.log(Array.from(text).slice(0, 6). join(""));
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.
@michael-0acf4
Good point... Each entry is parsed as a string on the rust side, so UTF validation might fail...
Unless the implementation follows the well-formed JSON.stringify
implementation; which we can assume...
Because the message should be a valid JSON...
…#904) - Send the JSON-RPC message is chunks in the TypeScript typegraph client to prevent reaching the line size limit for stdout. Note: we could not reproduce the issue locally as it only occurs when using the published package for Node.js. - Use JSON-RPC notification for logging and report from the typegraph clients. - Other changes: - Use relative paths for static task sources in the CLI; - Fix TODO in `meta gen`: pass the working directory on the `working_dir` param of `SerializeActionGenerator::new`. <!-- 2. Explain WHY the change cannot be made simpler --> <!-- 3. Explain HOW users should update their code --> #### Migration notes _N/A_ --- - [ ] The change comes with new or modified tests - [ ] Hard-to-understand functions have explanatory comments - [ ] End-user documentation is updated to reflect the change
meta gen
: pass the working directory on theworking_dir
param ofSerializeActionGenerator::new
.Migration notes
N/A