Based on API docs and ts definitions.
Depends on D6993
Details
Details
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
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? |
| 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. |
| 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. {
+done: boolean,
+value: ...,
};while for Iterator it is: | {
+done: true,
+value: ...,
}
| {
+done: false,
+value: ...,
}; |
| 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? |
Comment Actions
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. |
| 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 |