-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
Add @types/deno #70492
base: master
Are you sure you want to change the base?
Add @types/deno #70492
Conversation
@dsherret Thank you for submitting this PR! This is a live comment that I will keep updated. 1 package in this PR
Code ReviewsThis PR adds a new definition, so it needs to be reviewed by a DT maintainer before it can be merged. 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. InactiveThis PR has been inactive for 9 days. Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 70492,
"author": "dsherret",
"headCommitOid": "53c816747eaefba0c565d10858587aed59ebbac1",
"mergeBaseOid": "5f56dc762773f1213fda4248e37a261637527c33",
"lastPushDate": "2024-09-09T23:33:23.000Z",
"lastActivityDate": "2024-09-16T20:15:53.000Z",
"hasMergeConflict": false,
"isFirstContribution": false,
"tooManyFiles": false,
"hugeChange": true,
"popularityLevel": "Well-liked by everyone",
"pkgInfo": [
{
"name": "deno",
"kind": "add",
"files": [
{
"path": "types/deno/.eslintrc.json",
"kind": "package-meta",
"suspect": "edited"
},
{
"path": "types/deno/.npmignore",
"kind": "package-meta-ok"
},
{
"path": "types/deno/deno-tests.ts",
"kind": "test"
},
{
"path": "types/deno/index.d.ts",
"kind": "definition"
},
{
"path": "types/deno/package.json",
"kind": "package-meta-ok"
},
{
"path": "types/deno/tsconfig.json",
"kind": "package-meta",
"suspect": "not [the expected form](https://github.com/DefinitelyTyped/DefinitelyTyped#user-content-tsconfigjson) (check: `compilerOptions.lib.0`)"
}
],
"owners": [],
"addedOwners": [
"dsherret",
"bartlomieju"
],
"deletedOwners": [],
"popularityLevel": "Well-liked by everyone"
}
],
"reviews": [
{
"type": "stale",
"reviewer": "jakebailey",
"date": "2024-09-14T17:29:46.000Z",
"abbrOid": "53a9958"
},
{
"type": "stale",
"reviewer": "lucacasonato",
"date": "2024-09-12T17:07:21.000Z",
"abbrOid": "53a9958"
}
],
"mainBotCommentID": 2339330236,
"ciResult": "pass"
} |
🔔 @dsherret — you're the only owner, 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...) |
Any reason to put all of the types here versus publishing a package which is referenced here? It seems like this is all ambient. Up to you. |
@jakebailey what's the advantage of doing that? |
It's just so that you can avoid having to push code here all the time; |
@jakebailey 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? |
Yeah, I feel like that creates a bit of indirection and as you said this already requires a review. It is nice to get the tooling here. We do have https://www.npmjs.com/package/@deno/types that we published to once for a single user and I was planning on deprecating it after this. Side note: I'm actually surprised |
Dependencies are manually reviewed and put in a list on dt-tools, so it's not just anything. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marking as needs review for the version thing; the suffix will definitely not behave the way you'd think and it's a bug that the tests don't complain.
@dsherret One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you! |
@jakebailey 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? |
Personally it still seems concerning to me because there are dependencies of packages in |
The key factor is that we really only add deps which are also deps of the packages being typed, such that some sort of transitive compromise is no worse than if you hadn't installed the types at all. |
* was not found. | ||
* | ||
* @category Errors */ | ||
export class NotFound extends Error {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these error types are empty classes so are treated as the same type as Error
(same structure, same type, not nominal); is there some private prop/identifier that can be added to these to distinguish them, or is this intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instanceof Deno.errors.NotFound
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Er, right, they're just compatible in other ways: https://www.typescriptlang.org/play/?#code/MYGwhgzhAECyCeBRATsg9sgjNApgDwBccA7AExhXWWgG8BfAWAChRIYFKMAmXQk86J2r1mzUjlbIc0AGYBXYsAIBLNMWhyIOIZgAUOAFxwkqDJgCURgG5plpANyimm7aay7iOAO7GhXXebmQA
* | ||
* @default {false} | ||
*/ | ||
env?: "inherit" | boolean | string[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I would double check that you don't want env?: "inherit" | boolean | string[] | undefined
, for people using exactOptionalPropertyTypes
, especially on options bags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe adding it would add some confusion around what env: undefined
means. Generally people would just provide the properties they want to change here.
types/deno/index.d.ts
Outdated
@@ -0,0 +1,9756 @@ | |||
// Copyright 2018-2024 the Deno authors. MIT license. | |||
|
|||
/// <reference lib="dom" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What from the DOM is needed here? Usually requiring this isn't a good idea, rather, we can design packages to work with or without these and then have the DT infra test with multiple configurations (new to DT)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Request
, Response
, and URL
- but probably we shouldn't do this here I agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I shouldn't have added it. Tests seem to pass now without it though. Edit: Follow up here: #70492 (comment) (it's because of the tsconfig)
| "SIGXCPU" | ||
| "SIGXFSZ"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One day I'll find the time to get dprint to write:
| "foo"
| "bar"
;
@lucacasonato, @jakebailey 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? |
"strictFunctionTypes": true, | ||
"types": [], | ||
"lib": [ | ||
"DOM", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, because I had it here. I'll look into removing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, removing this requires a pretty significant change. Might be better to just require people to use "DOM" for now, then we can look into creating all the conditional types later so it works well with or without dom types.
@jakebailey, @lucacasonato 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? |
Are you expecting to have this out before Deno v2, or should this be waiting until then? |
It would be nice to land this before and then we can iterate on it. It's a brand new distribution form so probably not a bad thing to release it early. |
Adds
@types/deno
. This was previously done in #60557, but closed because at the time we didn't believe there was much use for it. Nowadays with Deno 2.0, it will be possible to use Deno without the Deno language server when only using npm packages via a package.json and so it might be useful to have types for deno available at@types/deno
.The types here are code generated by denoland/deno#25545
pnpm test <package to test>
.Select one of these and delete the others:
If adding a new definition:
.d.ts
files generated via--declaration
dts-gen --dt
, not by basing it on an existing project.tsconfig.json
should havenoImplicitAny
,noImplicitThis
,strictNullChecks
, andstrictFunctionTypes
set totrue
.