DefinitelyTyped
Add class types to emscripten, fix readable-stream
This PR updates some file system types for @types/emscripten by filling in some empty interfaces. This was done by observing various fs-related files in emscripten-core/emscripten/src.
@dr-vortex Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through.
This is a live comment which I will keep updated.
2 packages in this PR
Code Reviews
This PR can be merged once it's reviewed.
You can test the changes of this PR in the Playground.
Status
- ✅ No merge conflicts
- ✅ Continuous integration tests have passed
- ✅ All owners or a DT maintainer needs to approve changes which affect more than one package
- ✅ emscripten
- 🕐 readable-stream
All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes.
Diagnostic Information: What the bot saw about this PR
{
"type": "info",
"now": "-",
"pr_number": 69218,
"author": "dr-vortex",
"headCommitOid": "b1a6b5a0abb2e60c4a89d4511aa1b17f9c922e71",
"mergeBaseOid": "a099beff2d2c92582b126108a898e56ce17b34de",
"lastPushDate": "2024-03-31T00:45:58.000Z",
"lastActivityDate": "2024-05-13T22:47:46.000Z",
"maintainerBlessed": "Waiting for Code Reviews",
"mergeOfferDate": "2024-05-13T22:06:11.000Z",
"mergeRequestDate": "2024-05-13T22:47:46.000Z",
"mergeRequestUser": "dr-vortex",
"hasMergeConflict": false,
"isFirstContribution": true,
"tooManyFiles": false,
"hugeChange": false,
"popularityLevel": "Critical",
"pkgInfo": [
{
"name": "emscripten",
"kind": "edit",
"files": [
{
"path": "types/emscripten/index.d.ts",
"kind": "definition"
}
],
"owners": [
"zakki",
"periklis",
"kbumsik",
"lourd"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Critical"
},
{
"name": "readable-stream",
"kind": "edit",
"files": [
{
"path": "types/readable-stream/index.d.ts",
"kind": "definition"
}
],
"owners": [
"TeamworkGuy2",
"markdreyer"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Critical"
}
],
"reviews": [
{
"type": "approved",
"reviewer": "andrewbranch",
"date": "2024-05-13T22:05:31.000Z",
"isMaintainer": true
},
{
"type": "approved",
"reviewer": "lourd",
"date": "2024-05-13T19:31:47.000Z",
"isMaintainer": false
}
],
"mainBotCommentID": 2028513258,
"ciResult": "pass"
}
Hey @dr-vortex,
:unamused: Your PR doesn't modify any tests, so it's hard to know what's being fixed, and your changes might regress in the future. Please consider adding tests to cover the change you're making. Including tests allows this PR to be merged by yourself and the owners of this module.
This can potentially save days of time for you!
🔔 @zakki @periklis @kbumsik @lourd @TeamworkGuy2 @markdreyer — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.
This PR also fixes some types with readable-stream so that they work correctly with Node's ReadStream and WriteStream, that way it is possible to do:
import { Readable } from 'readable-stream';
import type { ReadStream } from 'node:fs';
class MyReadStream extends Readable implements ReadStream {
// ...
}
Re-ping @zakki, @periklis, @kbumsik, @lourd, @TeamworkGuy2, @markdreyer:
This PR has been out for over a week, yet I haven't seen any reviews.
Could someone please give it some attention? Thanks!
It has been more than two weeks and this PR still has no reviews.
I'll bump it to the DT maintainer queue. Thank you for your patience, @dr-vortex.
(Ping @zakki, @periklis, @kbumsik, @lourd, @TeamworkGuy2, @markdreyer.)
@andrewbranch,
Thank you for the review! This PR needed to fix types so I can work on @zen-fs/emscripten. I have some prototype code, however, the missing types on @types/emscripten prevent it from working. BrowserFS (what ZenFS is a fork of) implements these types on its own.
@andrewbranch Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?
I would really like emscripten types owners to weigh in. Pinging @zakki, @periklis, @kbumsik, @lourd one more time.
Pinging @kripken here, one of the Emscripten authors. Alon, could you help us find someone who could review this and generally double-check the existing types? I believe those of us who are considered owners on this were pretty casual contributors. I know I just added a few types for stuff I noticed missing, and that I don't have the context to be reviewing more additions effectively.
It would be ideal if someone from the core Emscripten team could help maintain these types. With TypeScript being a de facto industry standard now, people look at these types as official.
cc @brendandahl @sbc100 as maybe this can be connected to current TypeScript work in Emscripten?
Some of these could be auto generated with the new --emit-tsd <filename> option. It's going to be hard to generate a complete tsd file for emscripten though, since the output is highly dependent on what options are used and what is exported.
Didn't know about --emit-tsd, sounds like a great new feature! Which ones do you think shouldn't be included? How might we guide users towards using --emit-tsd as part of the types here? Maybe in the types' JSDoc comments? Being unknown with a tip to use --emit-tsd?
@dr-vortex Unfortunately, this pull request currently has a merge conflict 😥. Please update your PR branch to be up-to-date with respect to master. Have a nice day!
@lourd @kripken @brendandahl @andrewbranch Can we look at merging this soon?
@dr-vortex: Everything looks good here. I am ready to merge this PR (at b1a6b5a) on your behalf whenever you think it's ready.
If you'd like that to happen, please post a comment saying:
Ready to merge
and I'll merge this PR almost instantly. Thanks for helping out! :heart: