Skip to content

Correct 'BigInt' types to 'bigint' (and add Typescript checking script) #84

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

Merged
merged 2 commits into from
Sep 3, 2024

Conversation

jonvuri
Copy link
Contributor

@jonvuri jonvuri commented Sep 2, 2024

First off, fantastic work on this project! The recent addition of Typescript types is a massive feature, especially for such a complex and unorthodox (in JS) API.

I noticed one small glitch in them when introducing the latest version to my private project, where I had already begun using an older, less comprehensive type declaration and my own bigint types. The glitch is that BigInt types are used, but they should be bigint instead. It's a very confusing situation, but the PascalCased 'object types' for primitive values are actually just the type of the constructor function for the boxed object versions of the primitives. It's usually a mistake to use those as the type of the values themselves, and it can cause issues in usage, the main one being that BigInt and bigint are not assignable to each other (which makes sense, one is a number and one is a constructor function):
image

Some sources that explain this in some more detail:

This changeset fixes those types, and it also adds Typescript as a dev dependency, as well as a barebones tsconfig.json and a package.json script to run it and type-check index.d.ts. It wasn't failing before this change, but it's nice to have to make sure type changes don't introduce an internal error.

@sgbeal
Copy link
Collaborator

sgbeal commented Sep 2, 2024

... unorthodox (in JS) API.

For trivia's sake, an explanation of that aspect: non-vanilla JS has never been on our development radar in the upstream project, so the quirks of higher-level tools plays no role whatsoever in its shape. Our high-level APIs (namely the sqlite3.oo1 namespace) provide "a" solution which people can use if they like, while making no claim whatsoever about being "the" solution. We also provide very-close-to-C-native bindings from which more "downstream-idiomatic" solutions can be built, but we leave the creation of those to folks who live in that highly-fluid downstream ecosystem. We'd love to see other solutions evolve, but so far nobody has pulled on that thread.

Copy link
Collaborator

@tomayac tomayac left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@tomayac tomayac merged commit b3305ae into sqlite:main Sep 3, 2024
@tomayac
Copy link
Collaborator

tomayac commented Sep 3, 2024

Released as 3.46.1-build3.

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.

3 participants