Skip to content
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

move to jsr for dependencies and modernize some things #487

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

williamhorning
Copy link

@williamhorning williamhorning commented Aug 23, 2024

This pr renames the jsr package so that it uses the @db scope (used by the mongo and sqlite drivers), moves the dependency on the standard library over to the jsr ones, updates those dependencies, and replaces code that relies on deprecated functions. The following three tests are still failing but I'm not too sure quite how to fix them:

  • Aborts TLS connection when certificate is untrusted
  • Defaults to unencrypted when certificate is invalid and TLS is not enforced
  • Attempts reconnection on socket disconnection

I'd like to definitely fix the second test as that one isn't expecting an error and is getting an error, while the other two are failing due to errors not matching the ones expected.

@bombillazo
Copy link
Collaborator

merge main to your branch again

@williamhorning
Copy link
Author

williamhorning commented Dec 21, 2024

@bombillazo can you take a look at this again? I merged the branches again and added a few changes to deal with Deno 2 as well. Additionally, the tests which failed here passed locally, so it seems as though something is off with Github Actions

@@ -679,8 +683,7 @@ export class Connection {

const buffer = this.#packetWriter.addCString(query.text).flush(0x51);

await this.#bufWriter.write(buffer);
await this.#bufWriter.flush();
await this.#connWritable.write(buffer);
Copy link
Collaborator

@bombillazo bombillazo Jan 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we no longer flushing the writer?

Copy link
Author

@williamhorning williamhorning Feb 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we aren't flushing the writer anymore since the original code was flushing the data from the BufWriter class (from @std/io) to the underlying Deno.Writer, but since we're not using either of those and are using normal web streams here, awaiting the write seems to be fine in making sure everything gets written

Copy link
Collaborator

@bombillazo bombillazo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, but I have some minor questions.

@williamhorning
Copy link
Author

@bombillazo is there any chance you can run the workflows for the latest commit here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants