-
Notifications
You must be signed in to change notification settings - Fork 112
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
feat: add bun
web framework adapter
#583
Conversation
There currently is no attempt at being compatible with bun. Bun seems to work for the core library, but bun's compatibility with Node is rather bad, and several grammY plugins currently crash when used with bun. I am personally not interested in working on bun compatibility, and I'll leave it up to bun to improve their stability and Node compatibility. This means that if there are issues with bun compat, I will neither put any effort into this, nor am I going to spend time maintaining code that other people submit if its sole purpose is to work around issues in bun. Consequently, I would also reject such changes. We have a few options here:
I am afraid that people will build up false expectations about bun compatibilty if we have this adapter. Do you think that people will start making demands if we merge this? |
There's another option: https://t.me/grammyjs/237897 |
Сould you tell which ones exactly? I can try to make them compatible
Obviously a bad idea
That would be ok
I'm not sure what exactly you're suggesting. To release something like grammyjs-with-bun-web-adapter npm package? Or to make it a plugin? As far as I understand, the second is not possible |
i have no idea how to import bun types, really non-trivial task the only two ways (
|
@winstxnhdw has agreed to take care of the people who make demands. The affected plugins are i18n and conversions, and you cannot fix their compat because it's caused by the build tooling. We need to switch to JSR instead, which will solve all of this, but until then, we can already provide this adapter. Can you just copy the bun API types here, and rely on that? |
This comment was marked as outdated.
This comment was marked as outdated.
You have to narrow down the types. |
I doubt this is doable. I've tried everything |
I don't see any problem when I do it though? EDIT: Oh nvm I get it. |
test/convenience/webhook.test.ts
Outdated
@@ -1,4 +1,5 @@ | |||
/// <reference types="npm:@types/node" /> | |||
/// <reference types="npm:bun-types" /> |
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.
You may want to import this regularly instead of via reference, I'm not entirely sure if that's possible, but Node and Bun seem to have conflicting global types.
Perhaps it's even better to import both things via import statement. Something like const x: import("..").Bun = ..;
is what I have in mind. Hope it helps.
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.
Perhaps it's even better to import both things via import statement. Something like const x: import("..").Bun = ..; is what I have in mind.
Hmm, my intellisense gives me the correct type but the tests still thinks the types are "any".
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 am thinking of using a type declaration, but it's not being picked up in Deno. Does Deno not support 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.
A type declaration? What do you mean? type X = 0;
?
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.
No, like a type.d.ts file. I was thinking maybe I could isolate /// <reference types="npm:bun-types" />
into a type declaration, and rewrite the bun types for Bun.serve
.
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.
You don't need to contribute anything to Deno. You just have to use their LSP instead of tsserver.
um, how and where to use the deno lang server when running deno..?
i meant that deno does not use the ts lang server to resolve types, so it has its own logic, which is less future-proof, as we can see - ide can resolve bun's node-like imports, but deno cannot (most likely because of some special case only for imports starting from "node:")
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.
Can't we just model them in the same way we model everything else?
Speaking about the node ones - i don't think we can, since other used types (next js for example) also import them
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.
this probably happens because your ide uses the ts lang server to resolve types, but deno doesn't. I recently faced exactly the same issue with different product and bun because of this
nvm, i forgot what the original problem was when wrote that
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.
Why do we need the types from bun and node in the first place? Can't we just model them in the same way we model everything else?
Because these are basically integration tests. The point is that we use the types from their actual implementation (provided by the library authors) to test against our stubs. It'll remove the purpose of the test if we were to stub them as well. It would be the same as testing our code against itself.
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.
That is true, but as three pairs of eyes have reviewed this 3-line type now, I am fairly confident that we got it right :D
Also, Jarred cannot change it before Bun 2.0 as that would be breaking. If we ever find a way to import the types in an isolated way without polluting the global types, we can improve this code in the future. For now, I don't want that to block us further.
No description provided.