Page MenuHomePhabricator

[web] Add types for WASM File System API
ClosedPublic

Authored by kamil on Jul 19 2023, 2:27 AM.
Tags
None
Referenced Files
F2157913: D8544.diff
Mon, Jul 1, 2:48 AM
F2157742: D8544.id28892.diff
Mon, Jul 1, 2:28 AM
F2157619: D8544.id28933.diff
Mon, Jul 1, 2:11 AM
F2157618: D8544.id28893.diff
Mon, Jul 1, 2:11 AM
F2157617: D8544.id28892.diff
Mon, Jul 1, 2:11 AM
F2157616: D8544.id28815.diff
Mon, Jul 1, 2:11 AM
F2157615: D8544.id28814.diff
Mon, Jul 1, 2:11 AM
F2157583: D8544.diff
Mon, Jul 1, 2:08 AM
Subscribers

Details

Summary

Based on:

Test Plan

flow

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
web/database/types/file-system.js
45 ↗(On Diff #28814)

docs.

ops set of callbacks required by the device, docs provides an only example for one specific filesystem to write types to each separately will probably be an overkill

48–49 ↗(On Diff #28814)

docs

e in callback is en error, unfortunately in this case docs does not day what the error can be (basically everything, number with error code, JS Error or some object)

53 ↗(On Diff #28814)

docs

there is no definition of the return type, and in an example, the function does not return anything, but after reading the docs there are some sentences like: The parent folder, either as a path (e.g. ‘/usr/lib’) or an object previously returned from a FS.mkdir() etc. so I don't think there is a better way of specifying the type

kamil published this revision for review.Jul 19 2023, 8:56 AM

Personally I don't like the FS$ namespacing and don't think it's needed (it's namespaced to the the module/file anyway)

web/database/types/file-system.js
3–9 ↗(On Diff #28815)

I don't think this works. In the typescript types, they just have defined a few variables, but we don't know what values they have

20 ↗(On Diff #28815)
45 ↗(On Diff #28815)

If we don't use it, we don't have to type it more specificly, but we can use mixed instead of any without any loss

48–49 ↗(On Diff #28815)

This will make it a bit less ergonomic, but worst case, we can cast to any inside of the callback

57 ↗(On Diff #28815)

Should this be void?

103 ↗(On Diff #28815)

I think that based on the typescript types it should be like this (also in the other readFile and writeFile)

michal requested changes to this revision.Jul 20 2023, 7:26 AM
This revision now requires changes to proceed.Jul 20 2023, 7:26 AM

remove FS$ prefix

web/database/types/file-system.js
57 ↗(On Diff #28815)

I don't think so, look a this comment: https://phab.comm.dev/D8544?id=28814#inline-54859

At least I can update this to mixed

103 ↗(On Diff #28815)

right

This revision is now accepted and ready to land.Jul 21 2023, 4:38 AM
This revision was automatically updated to reflect the committed changes.

Interface is a weird construction in Flow... it's generally best to use type if possible. Is there a strong reason for using interface here?

I guess it matches TypeScript, and at this point it would probably be too much work to update it...

But in the future it's best to start with type if possible.

Interface is a weird construction in Flow... it's generally best to use type if possible. Is there a strong reason for using interface here?

I guess it matches TypeScript, and at this point it would probably be too much work to update it...

But in the future it's best to start with type if possible.

Yeah probably the only reason was that I tried to match TypeScript.

I created a follow-up ENG-4526. I'll update this when I will have any spare cycles.