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 inside the package #3

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

artospaj
Copy link

@artospaj artospaj commented Jan 1, 2021

I've done a TS mapping to be able to use the mqttsn-packet in my project and I'd like to share the results with you.

@artospaj
Copy link
Author

@ithinuel could you please take a look?

Copy link
Owner

@ithinuel ithinuel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution, I'm glad it can help you !

I don't know much about typescript so I have some questions:

  • Is there a way to test the validity of those new files ?
    eg, checking the APIs actually match and are not broken by futures changes ?
  • Can those test be tested in CI ?

Speeking of CI, I noticed that the nodejs versions in .travis.yml where long outdated. So I prepared #4 to bump them to latest version and latest LTS.

index.d.ts Outdated Show resolved Hide resolved
packet.d.ts Outdated Show resolved Hide resolved
@artospaj
Copy link
Author

@ithinuel thank you for pointing out the question regarding type validation, I've taken some time to verify what's possible and found out it can be partially done using:

npx tsc --allowJs --checkJs ./mqttsn.d.ts ./packet.d.ts

This led to few issues being fixed 🙇

Unfortunately, that's only a semantics and for sure it can let some logical errors pass. The only way I can know is to rewrite tests to TypeScript, to let compiler jump in when it detects any difference in types when API methods are used in test scenarios.

@ithinuel
Copy link
Owner

Thanks for looking into this.
Would you mind updating .travis.yml to add that check ?

Do you think it'd be reasonable to add a little .ts example to demo this lib using typescript ?
This could be run in CI to have at least a minimal typescript subset being tested.
EG: have SubscribeShortTopic turned into a Buffer and parsed back into an object ?

I could then use that as a reference if/when I find time to learn typescript 😄

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