-
Notifications
You must be signed in to change notification settings - Fork 14
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
feat: update to deno v2 #48
base: master
Are you sure you want to change the base?
Conversation
This is probably the thing I'm waiting for:
|
Thanks! I forgot to mention that I ran the workflow last week but it failed. I think it just needs to be updated to use deno v2. |
deno fmt and upgrade to v2 to make CI pass
@bramtechs Thank you for perfecting it ! I've merge it to my branch, now this should auto sync changes. let's wait @lenkan check it |
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.
Thanks a lot!
In general, it looks good to me! However, it would be good to make sure there is test coverage for the case when the connection reader does not read the entire buffer in a single call. I think it might cause issues (?). That was the reason the BufReader was used previously.
const buf = new Uint8Array(length); | ||
const n = await this.#reader.read(buf); | ||
|
||
if (n === null) { | ||
throw new FrameError("EOF"); | ||
} | ||
|
||
return n; | ||
return buf; |
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.
I am wondering if this might cause issues on slower connections. From the docs https://docs.deno.com/api/deno/~/Deno.Conn#call_signature_read_0:
It is possible for a read to successfully return with 0 bytes. This does not indicate EOF.
It is not guaranteed that the full buffer will be read in a single call.
These are the cases that BufReader handled. I see the tests passes though. But I do not remember if there is a test that specifically triggers the case where the full buffer is not read in a single call.
summary
BufReader
denoland/std#6056jsr:@std/io
v0.225 Breaking change #47