element-hq
Change `timeoutIdRef` type number to `ReturnType<typeof setTimout>`
Bug Overview
During the code check using the yarn lint script, the following error was found.
Solution
After looking into the cause, I found out that the browser return type of the setTimeout function is a number type, but in NodeJS, it is returned as a Timeout type.
Because of this, it worked normally in the browser environment, but an error occurred during lint verification.
Therefore, I used ReturnType to handle it so that it could pass lint verification without being affected by the environment.
The solution was completed after that.
- [x] Tests written for new code (and old code if feasible).
- N/A New or updated
public/exportedsymbols have accurate TSDoc documentation. - [x] Linter and other CI checks pass.
- [x] I have licensed the changes to Element by completing the Contributor License Agreement (CLA)
We use number for timeout IDs throughout the element web codebase as it only runs in browsers. That said, I'm also getting the same error so something must have changed to make this type check fail sometimes. The correct solution is probably to find out what that is, and then if the answer is to change the types, we should do it everywhere.
Hello. Thanks for your comment! I'm not sure if I understood you correctly. It seems that the lint result is different depending on whether it's a NodeJS environment or a browser environment. If you use setTimeout in a browser, the number type is returned as you said, so I think it's better to fix it to number in the code. However, since lint runs in NodeJS, I think that problem occurs. I have thought of two methods.
- Change Lint to run according to browser environment.
- Change the code as a whole like I did to prevent related errors in NodeJS and browser without modifying Lint. I think the 2nd method is not a very good method... I will look into it a little more based on the 1st method!
I looked at the tsconfig file and it seems like the settings are set to recognize based on the DOM, which is definitely interesting.... Let's find out more!
I think I found the cause!
If I install it excluding the development dependency, it works as tsconfig.json says.
However, if I set it including the development dependency, it seems to be recognized as a nodeJS environment due to a setTimeout conflict caused by @types/node.
To solve this problem... I think I have no choice but to use a method that explicitly specifies it using window (a method that changes the entire code base). Do you have a better idea?
Looking at other codes that used setTimeout, it seems that there are places where window.setTimeout is used globally and places where it is not used.
It seems that it was used selectively depending on the situation, so should I unify this?
Apologies, I went to ask the team about this and then forgot. I just brought it up again in our meeting and the solution suggested was to try overriding the return type of setTimeout (not window.setTimeout) to a specific, opaque type, eg. an interface with a single, underscored dummy member. We can then hopefully converge everything on using setTimeout.
I'm not sure if this is a piece of work you'd be willing to take on.
Hello.
Thank you for your reply.
I read your reply. If this PR is not a serious issue that needs to be addressed immediately within the team, would you mind if I take it on?
I have been using Element internally and would like to contribute this time if it is okay.
Also, I think it would be a good learning experience to look at the code while contributing.
I'm assuming work on this will happen on a different PR, so I'm going to close this (although I can re-open if you do use this one).
Hello. First of all, I would like to say that I am truly sorry. I said I would take charge, but then suddenly I got so busy that I couldn't start working on it. I am truly sorry that I couldn't tell you about this separately.
Not a problem, its just that open PRs come up in our weekly meeting so if they're still open so we look at how to move them along.