grammyjs / web-app

Building blocks for Telegram Web Apps.
MIT License
73 stars 6 forks source link

Types to `@types/` #1

Closed laosb closed 2 years ago

laosb commented 2 years ago

It seems this version rewrites telegram-web-app.js in TS, but I think it's possible Telegram decides to change its inner working without changing its API, which may breaks apps built with this library. Maybe providing .d.ts and continue to use official js from their CDN would make more sense?

KnorpelSenf commented 2 years ago

I agree that this is something to keep in mind. We're monitoring the CDN script automatically and are notified of changes to it. I also don't expect Telegram to update their apps on a breaking way, because it's hard to ensure everyone updates. Either way, I agree we should allow people to use this as a types-only dependency if desired.

Does it make sense to publish this under @types/, given that it covers a CDN script rather than an existing JS npm package?

Another advantage of providing a module is that we can allow people to import from it rather than polluting window. This is currently not implemented here, but it would certainly be worth considering, given that the apps will still be able to communicate with the web view in that case.

I'm not quite sure how relevant this is given we're talking just a few bytes, but usually it's advantageous to bundle things up and let the app load with fewer requests.

We have a number of options here:

This is very much not decided :)

What's your opinion?

laosb commented 2 years ago

There're precedents for publishing types of a CDN script to @types/, like @types/wechat-miniprogram, so I do think it will be just fine. The point is that if people choose to use Telegram CDN version, they can still use those types to help. Also, technically it's possible for Telegram to hijack their own domain in WebView and return a local copy of that script in future releases, which no doubt will be a lot faster than any other optimizations we web developers can do, so for my own bot I might stick to their CDN release.

Not polluting global scope is something to consider though. I don't have strong hygiene on that, given it didn't change any else other than window.Telegram. I do like the idea of importing than just stuck an object on window, which is something with modern web, Telegram could do (but seems not interested in, given the fact t.me/DurgerKingBot is written in jQuery, and it's possible that some Telegram Android users could been running very old system WebView).

So, all in all my opinion would be:

  1. a .d.ts defining only everything in Telegram
  2. a wrapper of some kind if possible, to export Telegram in current CDN script and avoid polluting global namespace, which should be doable and less manual than maintaining essentially a fork of the original script.

and then package them in two different ways:

That would be enough and still easy to use.

KnorpelSenf commented 2 years ago

I am convinced, that sounds like a good plan.

Would you be willing to contribute these changes and take over from here? It seems like you know much better than me how to turn this library into something great. I am already quite busy with the other dozen plugins or so. You could join the @grammyjs organisation and be granted push+admin permission to this repository.

laosb commented 2 years ago

My output capacity is currently limited by my day job and deadline of my graduation paper, so I can't promise any timeline for that, but I'm willing to help.

KnorpelSenf commented 2 years ago

That sounds fair. I can do some more implementation. So how about you stick with what you've been doing so far (testing, feedback, ideas) until your schedule frees up, and then we do the transfer? :)

In the meantime, I'll create the two packages as discussed above.

KnorpelSenf commented 2 years ago

The events received from the Telegram client seem to go through window. Hence, it does not look like we will be able to avoid polluting the global scope.

Regarding bundling: I understand that bundling leads to fewer requests and faster loading time. However, the point is to call ready as soon as possible. Wouldn't it be much faster to drop the bundling and to load ony this script via CDN, in order to call the method as early as possible?

If so, this would basically mean that we should encourage users to do this:

<script src="https://telegram.org/js/telegram-web-app.js"></script>
<script>window.Telegram.WebApp.ready()</script>

If users still prefer to import from a package in their remaining code base, we could simply do something like the following.

export const WebApp = window.Telegram.WebApp

However, I think there is not much of an advantage anymore to having a dedicated package. It may still be cleaner for users to import from a package rather than going via window, as it becomes more clear where the values come from. Also, types might "just work" because no @types/ dependency will have to be loaded explicitly. What is your opinion here?

Either way, I'd create a pull request with these files against https://github.com/DefinitelyTyped/DefinitelyTyped.

Does that look good to you?

laosb commented 2 years ago

I personally prefer using their original CDN script than bundling, so that looks fine to me.

Regarding that ready thing, I think Telegram's doc is a bit misleading here - as I understand it, ready tells the client to reveal the web view to the user. So at the angle of UX, it will make more sense to call ready as soon as your UI is ready, not just the CDN script. Since most developers today are gonna use some kind of client-side JS libs like React and Vue to render UI, that would not be same as "when the Telegram lib is loaded" (unless of course if they use SSR techniques or have traditional server-side templates).

KnorpelSenf commented 2 years ago

I see that point.

I created a PR now for the @types/ package. Could you review? https://github.com/DefinitelyTyped/DefinitelyTyped/pull/59978

KnorpelSenf commented 2 years ago

Note that I stripped the type definitions which are not documented. They were useful for a TypeScript port of the script. They are superfluous now.

KnorpelSenf commented 2 years ago

@laosb could you please leave your review at #2?