grammyjs / emoji

Helpers for sending emojis.
https://grammy.dev/plugins/emoji
MIT License
9 stars 2 forks source link

Refactoring #19

Closed niusia-ua closed 1 year ago

niusia-ua commented 1 year ago

The charset option caused a build error: error TS5101: Option 'charset' is deprecated and will stop functioning in TypeScript 5.5. Specify compilerOption '"ignoreDeprecations": "5.0"' to silence this error.

niusia-ua commented 1 year ago

I would have liked to add a few upfront micro-optimizations, but they go against your style and make the code a bit unclear 😅

KnorpelSenf commented 1 year ago

The charset option caused a build error: error TS5101: Option 'charset' is deprecated and will stop functioning in TypeScript 5.5. Specify compilerOption '"ignoreDeprecations": "5.0"' to silence this error.

It had to be removed from all grammY repos, it's good to do this here, too.

dcdunkan commented 1 year ago

I would have liked to add a few upfront micro-optimizations, but they go against your style and make the code a bit unclear 😅

@Nazar-Ant Feel free to add those if it improves the plugin. We'll see if it does affect the style and readability of the code after you add them. And if it's too much, we can revert the changes.

niusia-ua commented 1 year ago

In general, it is bad practice to use the function name() {} without using this. I don't remember exactly, but the reason there is to create a function context, which is a rather difficult operation for a runtime, although modern runtimes can skip this step, but the code has to run for a while to collect statistics. Therefore, it is better to replace all function name() {} with variables with arrow functions, which are inherently context-free.

niusia-ua commented 1 year ago

By the way, did you know that converting a list of emojis into a JSON object, which is then converted into a JS object (JSON.parse()), can speed up the launch of a bot by 1.7x (the exact numbers depend on the runtime and the system itself; I haven't tested this for our emoji list)? The reason is that it's easier for a runtime to parse JSON syntax than JS because of its considerable lightness.

https://v8.dev/blog/cost-of-javascript-2019#json

niusia-ua commented 1 year ago

If you still prefer a function name () {}, you should place it in a variable, because then the function object will be created lazily instead of instantly, which will delay a runtime-intensive operation. That is: function name () {} - intensive const name = function () {} - simple

dcdunkan commented 1 year ago

In general, it is bad practice to use the function name() {} without using this. I don't remember exactly, but the reason there is to create a function context, which is a rather difficult operation for a runtime, although modern runtimes can skip this step, but the code has to run for a while to collect statistics. Therefore, it is better to replace all function name() {} with variables with arrow functions, which are inherently context-free.

Interesting. I didn't know that. But I'll leave the confirmation to @KnorpelSenf because I don't much about anything :)

Even though generally we tend to use normal function declaration style on the top level across grammY repositories, this change is okay I guess.

dcdunkan commented 1 year ago

By the way, did you know that converting a list of emojis into a JSON object, which is then converted into a JS object (JSON.parse()), can speed up the launch of a bot by 1.7x (the exact numbers depend on the runtime and the system itself; I haven't tested this for our emoji list)? The reason is that it's easier for a runtime to parse JSON syntax than JS because of its considerable lightness.

https://v8.dev/blog/cost-of-javascript-2019#json

Another interesting thing that I didn't know!

Do you think we can use here if it'll improve the performance? But what about auto-completion though? Does that affect it?

niusia-ua commented 1 year ago

I think there will be problems with the automatic creation of types as it is now. You will have to create a new huge type for this.

You don't really have to worry about that. It won't play a significant role. Except that it will save a little bit of resources on serverless

niusia-ua commented 1 year ago

I'm just bragging about my knowledge. Recently, I came across a Ukrainian programmer who understands JS up to the point where it is converted into bytecode and executed on the hardware level. I follow his work quite closely. He is increasingly opening my eyes to how JS really works.

By the way, if you replace all let/const in your code with var (if you thought it was outdated, you are very wrong), you can significantly optimize the framework's performance. The reason is that let/const uses certain checks that are completely absent in var. It also turned out that the concept of var is very logical, and let/const is overused, which spoiled the expressiveness of the language.

But again, modern runtimes can optimize such things if you don't interfere with them, and it's very easy to interfere with them)))

dcdunkan commented 1 year ago

@KnorpelSenf, I don't see any breaking changes in this. What should we do about the next package versioning, given that the emoji list have been corrected and updated? I think we can make it a minor release, because now we at least leave the incorrect emoji names as it is.

KnorpelSenf commented 1 year ago

Then again, the current setup is very simple yet very reliable, and messing around with it to save a few ms at best once during startup might not be worth the headache. This is something for @dcdunkan to decide :)

dcdunkan commented 1 year ago

Then again, the current setup is very simple yet very reliable, and messing around with it to save a few ms at best once during startup might not be worth the headache. This is something for @dcdunkan to decide :)

I agree with you after reading your comment. I think the setup we currently have in this PQ is great and simple enough.