Page MenuHomePhabricator

[web-db] add `sql.js` types
ClosedPublic

Authored by kamil on Mar 20 2023, 9:06 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 8, 1:35 AM
Unknown Object (File)
Thu, Mar 7, 3:59 AM
Unknown Object (File)
Thu, Mar 7, 3:59 AM
Unknown Object (File)
Thu, Mar 7, 3:59 AM
Unknown Object (File)
Thu, Mar 7, 3:59 AM
Unknown Object (File)
Thu, Mar 7, 3:59 AM
Unknown Object (File)
Thu, Mar 7, 3:59 AM
Unknown Object (File)
Feb 20 2024, 6:39 AM
Subscribers

Details

Summary

Based on API docs and ts definitions.

Depends on D6993

Test Plan

yarn flow check

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
michal requested changes to this revision.Mar 22 2023, 4:45 AM

I've left some inline comments about the types themselves. More general notes:

  • sql.js is just a normal npm package correct? Can you create a stub with flow-typed create-stub sql.js and replace the generated types with your code (keep the flow-typed signature and flow-typed version comments), so we are consistent with the rest of the types
  • You typed everything as a global and so I don't think the initSqlJs function is actually typed in db-worker.js (the global initSqlJs is overridden by the imported one which is still typed as any). You can type this package as a module?
web/flow-typed/SQLiteJS.js
4 ↗(On Diff #23869)

Does this need to be inexact?

20–21 ↗(On Diff #23869)

Typescript uses typeof keyword for typing these fields. I think they are supposed to hold classes themselves and not instances so we will also need to use typeof but please correct me if I'm wrong.

25 ↗(On Diff #23869)

Can we type the config better? In the sql.js documentation, there's locateFile specified (and in the emscripten documentation there are even more fields but we probably don't need to add the all)

66 ↗(On Diff #23869)

In typescript, it's typed as both Iterable and Iterator. Can we do the same?

This revision now requires changes to proceed.Mar 22 2023, 4:45 AM
ashoat requested changes to this revision.Mar 22 2023, 2:48 PM
ashoat added inline comments.
web/flow-typed/SQLiteJS.js
1 ↗(On Diff #23869)

You are taking a very non-standard approach here. Can you please make this file look like the rest, instead of polluting the global namespace with type declarations?

Start with flow-typed stub. Pay attention to where the file is created and what the naming convention is. Note that all declarations are wrapped in a declare module.

web/database/worker/db-worker.js
51 ↗(On Diff #24017)

Because it was badly typed previously I needed to do some enhancement here to satisfy Flow.
It is refactored later in the stack anyway

web/flow-typed/SQLiteJS.js
1 ↗(On Diff #23869)

Yeah, I did an awful job here, I put up this diff too chaotically without checking what will be best.

Creating stub and wrapping everything in declare module as you suggested

4 ↗(On Diff #23869)

Hmm... that's how it is typed in typescript, but inexact it's not needed in a way we will use it so I'll update this to exact for now.

20–21 ↗(On Diff #23869)

good catch!

25 ↗(On Diff #23869)

Making this inexact type with locateFile funciton

66 ↗(On Diff #23869)

I'm not sure if there is a need for this.
Also there will be a problem because for Iterable the next() function is typed as:

{
  +done: boolean,
  +value: ...,
};

while for Iterator it is:

| {
  +done: true,
  +value: ...,
}
| {
  +done: false,
  +value: ...,
};
ashoat added inline comments.
web/flow-typed/npm/sql.js_vx.x.x.js
17–18 ↗(On Diff #24017)

You set these properties to be read-only, but the values are not. In general we usually want read-only values if we're using a read-only property

On the other hand, if this is only used as a return type, and we don't care about passing it around, AND we know that all of the objects returned here are created fresh every single time, then we could make this type non-readonly

27–28 ↗(On Diff #24017)

I think you can also use Class<> instead of typeof, which is slightly more Flow-like but it honestly doesn't really matter

44 ↗(On Diff #24017)

Why is this create_function instead of createFunction? I guess it's sql.js being inconsistent with naming?

web/flow-typed/npm/sql.js_vx.x.x.js
59 ↗(On Diff #24017)

Why is this synchronous? I would've expected an async call that returns a Promise, but I guess the call basically just calls the underlying SQLite function, and it's all in a web worker so it's okay?

address review

web/flow-typed/npm/sql.js_vx.x.x.js
17–18 ↗(On Diff #24017)

According to this comment: https://phab.comm.dev/D7118?id=23917#inline-46618 I wanted to make this non-read-only, I just forgot to update properties.
Objects are created fresh each time so I will make then non-readonly

44 ↗(On Diff #24017)

Yeah, that's the case, example inconsistency:

59 ↗(On Diff #24017)

sql.js API is synchronous because it’s based on the C API (basically SQLite engine compiled to WebAssymbly).

Only worker uses this API and I've implemented app<->worker connection using Promises so it will not be a problem

This revision is now accepted and ready to land.Mar 24 2023, 9:29 AM
This revision was automatically updated to reflect the committed changes.