bun icon indicating copy to clipboard operation
bun copied to clipboard

refactor(types): Improve `bun-types`

Open Didas-git opened this issue 1 year ago • 13 comments

This PR aims to improve bun-types both for users and maintainers.

The problem

Since 1.0.19 a lot of issues related to the types have emerged and some people are stuck with older versions of the types for this reason, this issues started to appear because of the huge changes and usage of @types/node under the hood.

Some of the changes made include a lot of duplication and unecessary hacks for the sake of declaration merging, this is a problem for maintainers because changing something you need to do it twice and for users this causes issues because the hacks that were applied do not work in all typescript versions.

Some ideas

Declaration merging

To fix the declaration merging issues the types would require massive changes, while this changes would not break current user code, they would be a massive PR to change all the hacks to proper declaration merging and following the new ts standards, i am willing and want to work on this but i would need green light from the maitainers since huge prs are hard to review.

Stricter/better types

Something that is requested quite a lot by more advanced typescript users is more strict and improved types. While there was an attempt at this, it was refused due to breaking user code, with that in mind, by using the opportunity of making the changes above listed, we could also introduce bun-types/strict. This subpackage would allow us to add opt in stricter types without massive changes, for the initial idea all it would be is 1 file with less than 70 lines just for the main changes people want.

Not bundling (Impl)

This is how im opening this pr because its the smallest and simplest change.

While people might think that bundling types is the way to go because there is less files there is something important we must keep in mind. We are not sending type definitions to browsers.

Type definitions are loaded by tss (typescript server) and stay in memory while needed/referenced by the project, the problem with bundling is that typescript is required to load everything even if you just need simple types.

Chunking

Another advantage of not bundling is that we can break our type definitions into smaller chunks, doing this would make maintaining the types way easier specially when it comes to the globals file.

Note

I want to know the maintainers opinion before moving forward with this pr, i dont want to spend time on a pr of this size for it to just get red lighted afterwards.

Didas-git avatar Jan 16 '24 02:01 Didas-git

Hi @Didas-git, thanks for working on this and writing up your thoughts!

Declaration merging

I think this is a good idea. Right now there is a lot of weirdness/brokenness around types we add additional properties to, for instance, WebSocket and Headers. You have the green light.

Stricter/better types

This is a cool idea. I think we should do it, but maybe after we get declaration merging, merged?

Not bundling

This seems fine. I'll merge these changes into the work I'm doing in: #8189

Electroid avatar Jan 16 '24 16:01 Electroid

Thanks for answering, i will start working on this asap. I will leave this draft for all the declaration merging fixes and afterwards open a new one to discuss implementing stricter types in more details.

As for removing bundling. I decided to add a build script so you dont have to manually change the version everytime, but it could be removed in favor of just manually bumping it like we do for bun, i will leave that for you guys to decide.

Didas-git avatar Jan 16 '24 16:01 Didas-git

Chunking

Another advantage of not bundling is that we can break our type definitions into smaller chunks, doing this would make maintaining the types way easier specially when it comes to the globals file.

now bun-types exports a single d.ts file types.d.ts, this approach makes my IDE WebStorm very laggy.

Please keep in mind, if we can do something about this issue.

I will suggest something like this, but you know better.

/// <reference types="bun-types/sqlite.d.ts" />
/// <reference types="bun-types/tty.d.ts" />
/// <reference types="bun-types/bun.d.ts" />
/// <reference types="bun-types/assert.d.ts" />

// ...etc

TiBianMod avatar Jan 17 '24 17:01 TiBianMod

Chunking

Another advantage of not bundling is that we can break our type definitions into smaller chunks, doing this would make maintaining the types way easier specially when it comes to the globals file.

now bun-types exports a single d.ts file types.d.ts, this approach makes my IDE WebStorm very laggy.

Please keep in mind, if we can do something about this issue.

I will suggest something like this, but you know better.

/// <reference types="bun-types/sqlite.d.ts" />
/// <reference types="bun-types/tty.d.ts" />
/// <reference types="bun-types/bun.d.ts" />
/// <reference types="bun-types/assert.d.ts" />

// ...etc

This is part of bundling which is already removed in this pr, chunking would only affect the globals file because the others are already in chunks.

Didas-git avatar Jan 18 '24 02:01 Didas-git

i do want someone to verify that this package, if published, would properly install and work in ts language server. i know you're a huge ts nerd so i doubt that mistake will be here, but it is important to triple check this.

I will do a lot of testing on my end before marking the PR as ready for review, but also having other people test it is good since there might be some version related bugs since som older typescript versions are not as nice with declaration merging

Didas-git avatar Jan 18 '24 02:01 Didas-git

@Electroid there are some things i would like to discuss.

  • There is a type that is exported called AnyFunction would you agree on just removing this alias and typing (...args: any[]) => any in the places that use (there arent many) it would help the api be more explicit and would have less useless exports.
  • Regarding bun:ffi there is a type called Pointer, im not sure it is what you guys want.

type Pointer = number & {} just means it is a non nullable number which in this case is the same as just number This means this is assignable to anything that accepts a number and any number can be assigned to this type.

What i think you guys want instead is: type Pointer = number & { [unique symbol]: "pointer" } or even type Pointer = number & { pointer: null } With the above examples or any iteration of them you can constrain the functions to only get exactly this type, which means the user needs to get the number from a function or cast to this type explicitly.

Didas-git avatar Jan 18 '24 03:01 Didas-git

& { [unique symbol]: "pointer" }

I like this.

I think we should remove AnyFunction

paperclover avatar Jan 19 '24 01:01 paperclover

@Electroid @paperdave Regarding __filename and __dirname. Do we really need to redefine them and mark them as deprecated? Typescript already handles the types for them depending if you are using cjs or mjs (and the settings that make those changes as well)

Didas-git avatar Jan 19 '24 06:01 Didas-git

@Electroid @paperdave Regarding __filename and __dirname. Do we really need to redefine them and mark them as deprecated? Typescript already handles the types for them depending if you are using cjs or mjs (and the settings that make those changes as well)

In Bun, those work in both mjs and cjs, so it may make sense to keep the overrides.

Electroid avatar Jan 19 '24 17:01 Electroid

In Bun, those work in both mjs and cjs, so it may make sense to keep the overrides.

I am aware, but, usually you want people to use the newer alternatives that will also work cross runtimes, and since it has been marked on the types as deprecated for a while i was wondering if its rly needed.

Didas-git avatar Jan 19 '24 17:01 Didas-git

In Bun, those work in both mjs and cjs, so it may make sense to keep the overrides.

I am aware, but, usually you want people to use the newer alternatives that will also work cross runtimes, and since it has been marked on the types as deprecated for a while i was wondering if its rly needed.

Fair enough, I think we can remove it then.

Electroid avatar Jan 19 '24 17:01 Electroid

About the proposed chunking idea:

I probably will scrap that idea, i though it could be separated in chunks due to the amount of declarations in there but turns out it just has like 30 of the same declaration over and over instead of doing it all in one, so instead of chuncking i will be merging them all and fixing all the duplication and incorrectly exposed types (the ones prefixed with _)

Didas-git avatar Jan 19 '24 17:01 Didas-git

@Electroid @paperdave I think the PR is ready to start reviewing. Its worth noting that the type tests do not work, and its not because of my changes they are broken in main and all the other branches as well.

Didas-git avatar Jan 21 '24 12:01 Didas-git

@Didas-git Looks like CI for types is failing due to TS issues, mind taking a look?

Electroid avatar Jan 23 '24 16:01 Electroid

@Didas-git Looks like CI for types is failing due to TS issues, mind taking a look?

They have been broken for a long time apparently. I commented about this some days ago:

Its worth noting that the type tests do not work, and its not because of my changes they are broken in main and all the other branches as well.

Didas-git avatar Jan 23 '24 17:01 Didas-git

We'll have to fix our CI later, thanks for working on this. Big improvement.

Electroid avatar Jan 23 '24 20:01 Electroid