-
Notifications
You must be signed in to change notification settings - Fork 122
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: integrate /types into core #720
Comments
I have a project where I only need the types. Now, granted, I know it's probably going to be possible to import them from the grammY package after this change, I think it's better to only fetch the types in a project that doesn't need the core package. PS: They're awesome btw, IMHO the best type definitions/utilities for the Bot API, at the very least in the TS world. |
@MobinAskari can you elaborate on this? Is this about saving the 200 KB of disk space? |
Not necessarily, but yes, it can be considered a benefit.
You're the creator of the package so of course you know this better than me, but I guess having them as a separate package will make things easier for other collaborators as well. The commit tree, PQs 😆 and issues are separated and IMHO more manageable. Of course this is subjective, for example companies like Vercel actually prefer to have everything in one repo to make maintenance easier. |
Those are some interesting points! It's a good thing we're discussing this.
This causes a lot of problems right now. grammY relies heavily on the types, so the two packages are tightly coupled. However, having a separate package means that people sometimes use different versions of grammY and the types together. We were able to fix many issues by completely disallowing any semver ambiguiity between grammY and its types via exact pinning of the patch version in this line Line 20 in b04bfee
grammy and @grammyjs/types side-by-side in the same project, and they add @grammyjs/types to their own package.json. This duplicates the dependency and causes incompatibilities again. We can solve all of this at once by inlining the types.
This used to be true for a very long time. Indeed, we shipped just declaration files in the beginning. However, some build tools struggled with .d.ts file handling, so we ended up converting them to .ts files with no runtime code and just type exports. Later on, we did end up adding runtime code in grammyjs/types#45. I was very reluctant to merge that (mainly for ideological reasons, and “purity,” whatever that may mean), but it unfortunately is very practical and solves problems that a bunch of people had. So yes, we do ship runtime code in that package for a while now. Tree-shaking is very easy to do by using
The import tree will stay the same. We are not going to export all the things from grammY directly. We are going to keep the In fact, the
I think this is going to stay the same (maybe I am misunderstanding this point right now)
I don't think I remember it … why would certain types not be accessible? Do you have a link to the issue for me (or a link to the message in the chat)? I am probably going to remember it again with more context |
Thank you for the thorough explanation. |
Oh that's you! I didn't map your TG to your GH account in my head yet |
It is a major PITA to have them in an external dep. It also has not helped in any way, ever. We should just inline the types.
We can then deprecate
@grammyjs/types
.The text was updated successfully, but these errors were encountered: