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

TypeScript support #5322

Closed
wants to merge 1 commit into from
Closed

Conversation

protyposis
Copy link
Contributor

Allow new code in TypeScript while keeping support for all existing JS files. This will help TS-experienced contributors to write code more cleanly and efficiently and allows to smoothly transition existing code to TS over time.

Uses the same TS config as https://github.com/Koenkk/zigbee-herdsman and introduces one breaking change (see README for details) that requires updating two imports in Z2M.

@Koenkk
Copy link
Owner

Koenkk commented Jan 14, 2023

Although I'm a big fan of TypeScript, I'm still wondering wether it is the correct choice for zigbee-herdsman-converters. The main point is that for adding new devices, people copy code from this repo to their external converters (which cannot support typescript). Thus it will be more complicated to do this, especially if you have no clue about TypeScript/JavaScript.

@protyposis
Copy link
Contributor Author

I thought so. Personally I'd love to be able to use TS and the advantages it brings, and I've been missing it in the other PRs I created, so I gave it a try. To make new TS-based code copyable, we could commit the compiled JS files as well. That's not pretty but serves this purpose.

Alternatively we could ditch the compile step altogether in all Z2M repos (except in CI for validation), publish packages with TS files and migrate to ts-node for execution? This would allow loading external converters from JS and TS files and everything would continue to work as-is, only with added TS support on top.

@Koenkk
Copy link
Owner

Koenkk commented Jan 15, 2023

we could commit the compiled JS files as well. That's not pretty but serves this purpose.

These are pretty unreadable, so not good solution IMO.

Alternatively we could ditch the compile step altogether in all Z2M repos (except in CI for validation), publish packages with TS files and migrate to ts-node for execution?

It's a long time ago that I checked ts-node, although I find using ts-node a bad solution (extra overhead), could we use it for just loading the external converters? If we can maintain .js compatibility of external converters I'm totally fine with using TypeScript for zigbee-herdsman-converters.

@protyposis
Copy link
Contributor Author

protyposis commented Jan 15, 2023

These are pretty unreadable, so not good solution IMO.

Since this package targets es2018, the only syntax I am aware of for which TS adds ugly code is import/export, which could be avoided by continued use of require calls. What's left is the way exports are written in transpiled code (separate export object assignments for every exported item instead of one cleanly structured object [usually at the bottom of a file]). The missing whitespaces could be easily fixed by running prettier on the transpiled files. But I agree, it's not great, and using ts-node would be cleaner.

although I find using ts-node a bad solution (extra overhead), could we use it for just loading the external converters?

The overhead that ts-node adds is a transpilation step at start (or more generally, TS file load), afterwards it's just JS code executed by node. So there is some memory overhead for tsc which can be minimized by disabling type checking (which is done as a pre-processing step by the CI and release process) as suggested in TypeStrong/ts-node#104.

To keep the current startup performance, we could still transpile all existing code to JS as currently done, and just execute it with ts-node. For the JS code (e.g. the Z2M packages), there shouldn't be a difference as ts-node just passes those files to node. But, whenever a TS file is loaded, e.g. an external converter written in TS, it would automatically transpile (with some memory overhead) to JS. This way, users could write external converters in JS and TS (and therefore copy JS and TS code from this repo), and the transpilation overhead would be optional and only happen if the converter is written in TS. A standard production setup would thus not suffer from any significant overhead. I think this could be a very clean and at the same time performance-optimized solution that brings this repo onto a new level in terms of developer experience without introducing tech debt.

Edit: Alternatively, a custom node hook could be written to transpile loaded TS files, e.g. as documented here, but it does not make sense to reinvent the wheel when ts-node provides exactly that - which can be clearly seen by the fact that ts-node is just a wrapper command for node -r ts-node/register. However, it adds some memory overhead (see below) which might be completely avoidable with an optimized custom implementation.

Edit 2: A quick test has shown 95 MB memory use after startup of an empty Z2M instance (Windows, Node 14, dummy COM port) vs. 135 MB (+40 MB) with ts-node without TS code. Startup time looks similar. Is this an acceptable trade-off for TS support for you?

@Koenkk
Copy link
Owner

Koenkk commented Jan 16, 2023

Edit 2: A quick test has shown 95 MB memory use after startup of an empty Z2M instance (Windows, Node 14, dummy COM port) vs. 135 MB (+40 MB) with ts-node without TS code. Startup time looks similar. Is this an acceptable trade-off for TS support for you?

This is definitely an acceptable trade-off. External converters should only be used when adding support for new devices, not long-term (and even then users still can write .js style external converters).

@nurikk @kirovilya what do you think about this?

@protyposis protyposis closed this Feb 15, 2023
@protyposis protyposis deleted the feature/typescript branch February 15, 2023 14:29
@Koenkk Koenkk mentioned this pull request May 20, 2023
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

Successfully merging this pull request may close these issues.

2 participants