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

Improve types #75

Merged
merged 3 commits into from
May 21, 2024
Merged

Improve types #75

merged 3 commits into from
May 21, 2024

Conversation

qwtel
Copy link
Contributor

@qwtel qwtel commented May 19, 2024

Improves some type info related to VFS

@@ -5729,7 +5737,7 @@ declare type CAPI = {
*
* See https://www.sqlite.org/c3ref/vfs_find.html
*/
sqlite3_vfs_find: (vfsName: string) => sqlite3_vfs;
sqlite3_vfs_find: (vfsName: string|null) => WasmPointer;
Copy link
Contributor Author

@qwtel qwtel May 19, 2024

Choose a reason for hiding this comment

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

find doesn't return the vfs directly, needs a separate new sqlite_vfs(ptr) call.
Also, it's allowed to pass null to get the default vfs

@@ -2032,7 +2032,15 @@ declare type Sqlite3Static = {
installVfs: (obj: {
io?: {
struct: sqlite3_io_methods;
methods: Omit<sqlite3_io_methods, 'iVersion'>;
methods: { [K in keyof sqlite3_io_methods as K extends `x${string}` ? K : never]?: sqlite3_io_methods[K] };
Copy link
Contributor Author

@qwtel qwtel May 19, 2024

Choose a reason for hiding this comment

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

I think it's better to pick all properties that start with x (e.g. xRead, xWrite, etc) rather than omit known static properties, specially for sqlite_vfs below which has a lot more of them.

@tomayac
Copy link
Collaborator

tomayac commented May 21, 2024

@sgbeal Since this touches deeply upon SQLite internals, are you able to work with @qwtel to get this merged?

@sgbeal
Copy link
Collaborator

sgbeal commented May 21, 2024

i'm not clear how i can help here - this is all part of the TypeScript stuff, whereas we use only vanilla JS in the core. :-?

@tomayac
Copy link
Collaborator

tomayac commented May 21, 2024

Having TypeScript background would help (I'm not deeply into it either), but maybe some of the comments could help you as the implementer make sense of it?

// Give the `sqlite3_vfs_find` function an argument
// `vfsName` that's either a string, or null, and the result 
// will be a `WasmPointer`.
sqlite3_vfs_find: (vfsName: string|null) => WasmPointer;

Else, we can also just "YOLO-merge" it and hope for the best :-)

@sgbeal
Copy link
Collaborator

sgbeal commented May 21, 2024

i'm not at all clear what should be merged or what the patch is about. All i see is some typescript stuff and what might be a suggestion that we rename VFS properties in the core (which would break backwards compatibility, so won't happen). :-?

@tomayac
Copy link
Collaborator

tomayac commented May 21, 2024

@GabrielDelepine or @jbaiter from #54 might be able to review this?! Else, I'll just "YOLO-merge" it.

Copy link
Contributor

@jbaiter jbaiter left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏾

@qwtel
Copy link
Contributor Author

qwtel commented May 21, 2024

i'm not at all clear what should be merged or what the patch is about. All i see is some typescript stuff and what might be a suggestion that we rename VFS properties in the core (which would break backwards compatibility, so won't happen). :-?

There is no proposal to rename VFS properties in this PR, it is all based on how the API works as of 3.45.3.

The proposal is related to expressing the type of the methods property by using arcane typescript features, which is understandably un-reviewable by anyone who hasn't suffered through their share of TypeScript.

@tomayac
Copy link
Collaborator

tomayac commented May 21, 2024

Brilliant, thanks for the PR, @qwtel, and the review, @jbaiter!

@tomayac tomayac merged commit 58f75fc into sqlite:main May 21, 2024
@tomayac
Copy link
Collaborator

tomayac commented May 21, 2024

Published as 3.45.3-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.

4 participants