-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
[yauzl-promise] adjust types following v4 rewrite #69641
[yauzl-promise] adjust types following v4 rewrite #69641
Conversation
@LucVidal360 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. 1 package in this PRCode ReviewsBecause you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it. You can test the changes of this PR in the Playground. Status
Once every item on this list is checked, I'll ask you for permission to merge and publish the changes. Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 69641,
"author": "LucVidal360",
"headCommitOid": "f5b4bb0bfb375f40e437107f33ea202c4b05d6e8",
"mergeBaseOid": "425f0ebe145ac9337b0bcbd1084a548d729b4963",
"lastPushDate": "2024-05-18T21:08:09.000Z",
"lastActivityDate": "2024-05-18T21:08:09.000Z",
"hasMergeConflict": false,
"isFirstContribution": true,
"tooManyFiles": false,
"hugeChange": false,
"popularityLevel": "Popular",
"pkgInfo": [
{
"name": "yauzl-promise",
"kind": "edit",
"files": [
{
"path": "types/yauzl-promise/index.d.ts",
"kind": "definition"
},
{
"path": "types/yauzl-promise/yauzl-promise-tests.ts",
"kind": "test"
}
],
"owners": [],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Popular"
}
],
"reviews": [],
"mainBotCommentID": 2119003780,
"ciResult": "pass"
} |
🔔 @LucVidal360 — there are no owners, but it would still be good if you find someone to review this PR in the next few days, otherwise a maintainer will look at it. (And if you do find someone, maybe even recruit them to be a second owner to make future changes easier...) |
Please fill in this template.
pnpm test <package to test>
.Select one of these and delete the others:
If changing an existing definition:
package.json
.Description
Starting v4.0.0,
yauzl-promise
was rewritten from scratch. Cf. overlookmotel/yauzl-promise@v3.0.0...v4.0.0As such, several class properties were deleted/added/modified.
This PR is an attempt to fix the types to match the current implementation.
Exported properties
I tried to reflect only the officially documented properties, even though several more exist in the implementation.
However, I left undocumented properties if they were already exported in DefinitelyTyped, to avoid unnecessary breaking changes.
comment
andfilename
:string
orBuffer
?I've exported
ZipFile.comment
,Entry.comment
andEntry.filename
asstring
.However, if the
decodeStrings
option is explicitly set tofalse
, these 3 properties will effectively beBuffer
s.string | Buffer
, but I suppose most of the library users use the default option, and expectstring
s.ZipFile
andEntry
generic classes, depending on the type ofdecodeStrings
, but it felt overkill here, adding complexity.jsdoc
entry on the offending properties, to warn users of this fact.