-
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
Add types for bun.js, a new JavaScript runtime #59670
Conversation
@Jarred-Sumner 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 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 31 days — it is considered abandoned, and therefore closed! Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 59670,
"author": "Jarred-Sumner",
"headCommitOid": "28b6bee3cb19d6c7038ec2418efbd27ade99bdf3",
"mergeBaseOid": "7de5a3274b57a5602670134ef0a36fd4f17fa179",
"lastPushDate": "2022-04-04T00:42:15.000Z",
"lastActivityDate": "2022-04-14T20:05:41.000Z",
"hasMergeConflict": false,
"isFirstContribution": true,
"tooManyFiles": false,
"hugeChange": true,
"popularityLevel": "Well-liked by everyone",
"pkgInfo": [
{
"name": "bun",
"kind": "add",
"files": [
{
"path": "types/bun/bun-tests.ts",
"kind": "test"
},
{
"path": "types/bun/index.d.ts",
"kind": "definition"
},
{
"path": "types/bun/tsconfig.json",
"kind": "package-meta-ok"
},
{
"path": "types/bun/tslint.json",
"kind": "package-meta",
"suspect": "not [the expected form](https://github.com/DefinitelyTyped/DefinitelyTyped#user-content-linter-tslintjson) (check: `rules`)"
}
],
"owners": [],
"addedOwners": [
"Jarred-Sumner"
],
"deletedOwners": [],
"popularityLevel": "Well-liked by everyone"
}
],
"reviews": [
{
"type": "changereq",
"reviewer": "jakebailey",
"date": "2022-04-14T20:05:41.000Z"
}
],
"mainBotCommentID": 1086869775,
"ciResult": "pass"
} |
🔔 @Jarred-Sumner — 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...) |
@Jarred-Sumner The CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review. |
@Jarred-Sumner The CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review. |
@Jarred-Sumner The CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review. |
the project url seems to be wrong (at least for me it gives me 404 on GitHub). |
It is not yet a public repo but it will be soon Anyone can get access by going to bun.sh/discord, typing “I want Bun” and clicking the GitHub repo invite link the bot generates |
@peterblazejewicz what's the next step to get this merged? Sent you an invite to the repo. If you're not comfortable with merging this until the repository is public, then I can go ahead and make it public. |
Can you please generate a link for someone here to review the code? thx! |
Here you go:
Note that roughly none of the types listed here actually correspond to code in JavaScript or TypeScript. Bun is written in Zig and C++, and so are the runtime APIs. There are examples that use the APIs/types in https://github.com/Jarred-Sumner/bun/tree/main/examples and in https://github.com/Jarred-Sumner/bun/tree/main/integration/bunjs-only-snippets |
Just for reference, the docs for that dtslint rule is here. The version number check should work because the In this case, I think things are getting quite confused thanks to the fact that you're using someone else's existing package name for these types, when that package is still published on npm (not to mention has importers). The same lint override is used in a number of other packages, so that's not unprecedented. I know enough Zig and C++ to probably review this monstrous PR, but, I do sort of worry about taking over a package like this, especially when these types declare ambient stuff like other runtime-y declarations. |
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 is the plan for that npm package? Is it just going to be deprecated and made empty such that this types package can exist? Or is it going to be where the bun binary is published?
If the latter, you're going to end up having a problem because the types will bring in ambient types even though they're not intended to be used like that.
Perhaps what you need to do is have a package bun-env
instead that defines this environment, similar to webpack-env
(which works similarly).
Regarding the code itself, if you're only intending on having ambient types, then I don't think all of the exports are needed, as there's no actual import one can make directly (as you've noted). The dtslint warning can be silenced by noting that it's a non-npm package, so that's OK.
@Jarred-Sumner 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! |
@Jarred-Sumner I haven't seen any activity on this PR in more than three weeks, and it still has problems that prevent it from being merged. The PR will be closed on May 14th (in a week) if the issues aren't addressed. |
@Jarred-Sumner To keep things tidy, we have to close PRs that aren't mergeable and don't have activity in the last month. No worries, though — please open a new PR if you'd like to continue with this change. Thank you! |
Bun is a new JavaScript runtime environment, npm package manager, TypeScript & JSX transpiler, JavaScript & CSS bundler, and npm
package.json
scripts runner all in one incredibly fast CLI.npm test <package to test>
.If adding a new definition:
.d.ts
files generated via--declaration
dts-gen --dt
, not by basing it on an existing project.tslint.json
should contain{ "extends": "@definitelytyped/dtslint/dt.json" }
, and no additional rules.tsconfig.json
should havenoImplicitAny
,noImplicitThis
,strictNullChecks
, andstrictFunctionTypes
set totrue
.This does conflict with an existing npm package, however I am one of the collaborators of that npm package. That npm package will eventually be migrated to bun.
The
path
andfs
modules are mostly copied from@types/node
. Bun has native implementations ofnode:fs
andnode:path
that closely mirrors Node's API.Some of
index.d.ts
was copied from libdom.d.ts, but I have added more comments where relevant.I tried to avoid modifying the
tslint.json
but"npm-naming"
is disabled because bun doesn't export anything currently, unless I'm misunderstanding?If there is a way to avoid modifying
tslint.json
please do say