Skip to content
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

Open
Tracked by #675
KnorpelSenf opened this issue Dec 15, 2024 · 6 comments
Open
Tracked by #675

feat: integrate /types into core #720

KnorpelSenf opened this issue Dec 15, 2024 · 6 comments

Comments

@KnorpelSenf
Copy link
Member

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.

@MobinAskari
Copy link

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.

@KnorpelSenf
Copy link
Member Author

I think it's better to only fetch the types in a project that doesn't need the core package.

@MobinAskari can you elaborate on this? Is this about saving the 200 KB of disk space?

@MobinAskari
Copy link

MobinAskari commented Jan 8, 2025

I think it's better to only fetch the types in a project that doesn't need the core package.

@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.
Some other benefits I can think of:

  • it's already a separate package
  • it's not bundled with any runtime code, so the import and tree shaking are a lot easier and faster (though negligible)
  • it's also easier to navigate/traverse, and the import tree or intellisense gets less convoluted
  • it makes more sense from a naming perspective, having "grammy" or "grammy/types" in package.json signal different things to the collaborators of the project, especially in a big one
  • I'm not sure if you remember my situation or not, but different tsconfigs have different approaches to type imports, and in some scenarios, some types were not accessible at all. This will probably improve in v2, but things don't always end up in a nice spot and there might be temporary workarounds and type placements, which could break the imports.

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.

@KnorpelSenf
Copy link
Member Author

KnorpelSenf commented Jan 8, 2025

Those are some interesting points! It's a good thing we're discussing this.

  • it's already a separate package

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

"@grammyjs/types": "3.18.0",
in package.json. Unfortunately, contrary to what we have documented, many people use 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.

  • it's not bundled with any runtime code, so the import and tree shaking are a lot easier and faster (though negligible)

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 import type declarations. Most people do this anyway, so the imports are gone after the compile step and before bundling even begins. This is going to stay exactly the same after inlining the types.

  • it's also easier to navigate/traverse, and the import tree or intellisense gets less convoluted

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 /types export. Not cluttering the API reference/the list of exported symbols is important, and it would not make sense to let things get worse here.

In fact, the @grammyjs/types package splits the types in a somewhat arbitrary fashion across 10 or so files. I find them pretty hard to work with. When I'm done here, this problem will go away. In other words, the imports are actually going to get a lot easier. To be fair, these same changes could be applied without inlining the types, though. My point is that it won't get harder to work with them.

  • it makes more sense from a naming perspective, having "grammy" or "grammy/types" in package.json signal different things to the collaborators of the project, especially in a big one

I think this is going to stay the same (maybe I am misunderstanding this point right now)

  • I'm not sure if you remember my situation or not, but different tsconfigs have different approaches to type imports, and in some scenarios, some types were not accessible at all. This will probably improve in v2, but things don't always end up in a nice spot and there might be temporary workarounds and type placements, which could break the imports.

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

@MobinAskari
Copy link

Those are some interesting points! It's a good thing we're discussing this.

  • it's already a separate package

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

"@grammyjs/types": "3.18.0",

in package.json. Unfortunately, contrary to what we have documented, many people use 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.

  • it's not bundled with any runtime code, so the import and tree shaking are a lot easier and faster (though negligible)

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 import type declarations. Most people do this anyway, so the imports are gone after the compile step and before bundling even begins. This is going to stay exactly the same after inlining the types.

  • it's also easier to navigate/traverse, and the import tree or intellisense gets less convoluted

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 /types export. Not cluttering the API reference/the list of exported symbols is important, and it would not make sense to let things get worse here.

In fact, the @grammyjs/types package splits the types in a somewhat arbitrary fashion across 10 or so files. I find them pretty hard to work with. When I'm done here, this problem will go away. In other words, the imports are actually going to get a lot easier. To be fair, these same changes could be applied without inlining the types, though. My point is that it won't get harder to work with them.

  • it makes more sense from a naming perspective, having "grammy" or "grammy/types" in package.json signal different things to the collaborators of the project, especially in a big one

I think this is going to stay the same (maybe I am misunderstanding this point right now)

  • I'm not sure if you remember my situation or not, but different tsconfigs have different approaches to type imports, and in some scenarios, some types were not accessible at all. This will probably improve in v2, but things don't always end up in a nice spot and there might be temporary workarounds and type placements, which could break the imports.

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.
As for the last point, I was referring to this: https://t.me/grammyjs/287866

@KnorpelSenf
Copy link
Member Author

Oh that's you! I didn't map your TG to your GH account in my head yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants