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 Port #52

Open
deinok opened this issue Apr 10, 2018 · 6 comments
Open

TypeScript Port #52

deinok opened this issue Apr 10, 2018 · 6 comments

Comments

@deinok
Copy link

deinok commented Apr 10, 2018

Hi, any intention to port this to TypeScript?
We could benefit from Async/Await and other ES2015 features while mantaining compatibility with ES3 and the actual API of nodeS7.

Also, if interested I can work on this.

@deinok deinok mentioned this issue Apr 10, 2018
@Lichtjaeger
Copy link

Lichtjaeger commented Apr 10, 2018

This is only my opinion but I really don't see a reason to convert this project to typescript. Publish a type definition would make sense, but this is a completely different story.

This project needs a rewrite to at least ES2017 but you can use (for example) the babel transpiler for backward compatibility. Or limit the use to engines >= LTS.

@deinok
Copy link
Author

deinok commented Apr 10, 2018

With TypeScript you can transpile to ES3 like with babel. But with TypeScript you already have the definitions for TypeScript and also you have compile time protection about the usage of types.

@Lichtjaeger
Copy link

You can also autogenerate the type definitions from JS and type protection means nothing. They don't replace unit tests.

Also, you are transferring the build time to the end user. This results in a bigger memory usage and a longer installation time. The published package should be as small and fast as possible. In your PR you used the wrong hooks and I don't see an optimized .npmignore file.

@deinok
Copy link
Author

deinok commented Apr 10, 2018

Well, I didn't do the PR as a final port, only as an experimentation of the concept

@plcpeople
Copy link
Owner

Thanks for the PR. Sorry it took me so long to respond to it. I was able to get this working, although I have not run a lot of tests with real PLCs yet.

I am hesitant to merge the PR - we don't have a lot of experience with TypeScript development, and it's a major change for many people who contribute to (or simply use) nodeS7. I am not sure how this would fit with some major projects that use nodeS7, like the node-red wrappers.

Issue #41 has some discussion of a roadmap to 1.0, and if future major versions are going to use TypeScript, I'd like to see that discussion there.

@deinok
Copy link
Author

deinok commented Jun 17, 2018

Well, normaly S7 is used in Industrial environments. Where safety is the way to go. (Types of TypeScript).
About compatibility, using TypeScript dont require any change to any wraper. (If we dont change the public API of this lib)

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

3 participants