Skip to content

Conversation

littledivy
Copy link
Member

@littledivy littledivy commented May 21, 2025

Changes:

  • Set #[undefined] retval for open, set_allow_bare_named_parameters and set_read_bigint
  • Make readOnly property optional in DatabaseSync
  • Rewrite error handling and return Node error objects with code properties.

Todo:

@littledivy littledivy marked this pull request as ready for review May 29, 2025 16:00
ENABLE_DOUBLE_QUOTED_STRING_LITERALS_STRING = "enableDoubleQuotedStringLiterals",
}

let open_string = OPEN_STRING.v8_string(scope).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated to this PR, but I think the API should be OPEN_STRING.from_scope(scope) - the unwrap shouldn't be necessary here

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

A few comments but overall LGTM - really solid work

Comment on lines +33 to +37
suite('StatementSync.prototype.get()', () => {
test('executes a query and returns undefined on no results', (t) => {
const db = new DatabaseSync(nextDb());
t.after(() => { db.close(); });
let stmt = db.prepare('CREATE TABLE storage(key TEXT, val TEXT)');
Copy link
Member

Choose a reason for hiding this comment

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

Just to be 100% sure - have you verified that these actually pass and are not silently ignored by deno test is something goes wrong? If so, our support for node:test is getting pretty good 💪

Copy link
Member Author

@littledivy littledivy May 30, 2025

Choose a reason for hiding this comment

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

yep, went through fixing each test as the previous one passed so it does actually work!

@littledivy
Copy link
Member Author

littledivy commented May 30, 2025

Something weird happening in this new test file (parallel/test-sqlite-session.js):

running 14 tests from ./test/parallel/test-sqlite-session.js
creating and applying a changeset ... ok (0ms)
database.createSession() - closed database results in exception ... ok (0ms)
session.changeset() - closed database results in exception ... ok (0ms)
database.applyChangeset() - closed database results in exception ... ok (0ms)
database.createSession() - use table option to track specific table ... ok (0ms)
conflict resolution ...
  database.applyChangeset() - SQLITE_CHANGESET_CONFLICT conflict with default behavior (abort) ... ok (5ms)
  database.applyChangeset() - SQLITE_CHANGESET_CONFLICT conflict handled with SQLITE_CHANGESET_ABORT ... ok (4ms)
  database.applyChangeset() - SQLITE_CHANGESET_DATA conflict handled with SQLITE_CHANGESET_REPLACE ... ok (4ms)
  database.applyChangeset() - SQLITE_CHANGESET_NOTFOUND conflict with SQLITE_CHANGESET_OMIT ... ok (3ms)
  database.applyChangeset() - SQLITE_CHANGESET_FOREIGN_KEY conflict ... ok (3ms)
  database.applyChangeset() - SQLITE_CHANGESET_CONSTRAINT conflict ... ok (2ms)
  conflict resolution handler returns invalid value ... ok (2ms)
  conflict resolution handler throws ... ok (0ms)
conflict resolution ... ok (12ms)
database.createSession() - filter changes ... ok (0ms)
database.createSession() - specify other database ... ok (0ms)
database.createSession() - wrong arguments ... ok (0ms)
database.applyChangeset() - wrong arguments ... ok (0ms)
session.patchset() ... ok (0ms)
session.close() - using session after close throws exception ... ok (0ms)
session.close() - after closing database throws exception ... ok (0ms)
session.close() - closing twice ... ok (0ms)

"parallel/test-sqlite-session.js" failed:



You can repeat only this test with the command:

  ./target/debug/deno test --config tests/config/deno.json -A tests/node_compat/test.ts -- parallel/test-sqlite-session.js

All tests pass but its still reporting as failed with no output. I've reverted the test for now from CI.

littledivy added a commit that referenced this pull request Jun 2, 2025
Extracted from #29404

Upgrades deno_core to 0.350.0
littledivy added a commit that referenced this pull request Jun 2, 2025
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

Let's go

@littledivy littledivy merged commit fc02cf6 into denoland:main Jun 2, 2025
18 checks passed
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