Skip to content

chore(mqttverifier): typescript support#677

Closed
carlocorradini wants to merge 1 commit intobytebeamio:mainfrom
carlocorradini:mqttverifier
Closed

chore(mqttverifier): typescript support#677
carlocorradini wants to merge 1 commit intobytebeamio:mainfrom
carlocorradini:mqttverifier

Conversation

@carlocorradini
Copy link
Contributor

Related to PR #671

What has been changed:

  • rumqttverifier sources are now based on Typescript for better maintainability and more.
    Simply run npx ts-node path/to/script.ts instead of old node path/to/script.js

@de-sh de-sh requested a review from h3nill August 8, 2023 05:17
Copy link

@h3nill h3nill left a comment

Choose a reason for hiding this comment

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

i would prefer to keep these scripts in javascript because they are very occasionally used and having them to be typescript adds dependency on npx to run it while not providing much value to us.

also i have had very bad experience with trying to run typescript without build step (i.e. using ts-node)

currently as well running trying to run this fails:

TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" for /home/henil/Workspace/bytebeam/rumqtt/utils/mqttverifier/printer.ts
    at new NodeError (node:internal/errors:405:5)
    at Object.getFileProtocolModuleFormat [as file:] (node:internal/modules/esm/get_format:99:9)
    at defaultGetFormat (node:internal/modules/esm/get_format:142:36)
    at defaultLoad (node:internal/modules/esm/load:91:20)
    at nextLoad (node:internal/modules/esm/hooks:733:28)
    at load (/home/henil/.npm/_npx/1bf7c3c15bf47d04/node_modules/ts-node/dist/child/child-loader.js:19:122)
    at nextLoad (node:internal/modules/esm/hooks:733:28)
    at Hooks.load (node:internal/modules/esm/hooks:377:26)
    at MessagePort.handleMessage (node:internal/modules/esm/worker:168:24)
    at [nodejs.internal.kHybridDispatch] (node:internal/event_target:778:20) {
  code: 'ERR_UNKNOWN_FILE_EXTENSION'
}

@carlocorradini
Copy link
Contributor Author

@henil I disagree.
Having Typescript allows for better maintainability and type checking (I've found an invalid parameter thanks to TS and typings).
ts-node is a very established technology and used in small as well high value/big projects.
Moreover I've got a pretty good experience with Typescript/Ts-Node and the overall JS ecosystem :)... Never coming back to pure JS again ahaha

Are you using Node.js 20x? Note that it's not an LTS and currently it has some problems with Typescript. I've added in package.json the engines parameter to check this type of issues. Note that the CI is using 18.x

@h3nill
Copy link

h3nill commented Aug 8, 2023

$ npx --version
9.8.1
$ node --version
v20.5.0
$ npx ts-node --version
v10.9.1

this are the versions i am using.

Also i don't want to populate root of the project with npm related files.

@carlocorradini
Copy link
Contributor Author

You are using node 20.x which is not LTS (it will be in ~October)
I can change the directory of tsconfig to utils :)

@carlocorradini
Copy link
Contributor Author

Try on node 18.x (latest LTS) and It works flawlessly

@h3nill
Copy link

h3nill commented Aug 8, 2023

its weird that it works on older versions but not the lastest...

@carlocorradini
Copy link
Contributor Author

See TypeStrong/ts-node#1997

They will fix it.

Note also that until Node 20x is not LTS, it can be considered as an Alpa/Beta/Unstable release.

@h3nill
Copy link

h3nill commented Aug 8, 2023

Understood and I agree that TS is great, but this util scripts are only used to verify that bytes sequence for a particular packet generated by our protocol implementation matches with some other implementation (we were even considering to remove them because we think our implementation mature now). I don't really want to think about TS related things while doing it.

I've found an invalid parameter thanks to TS and typings

thats a bug that should be fixed but not something that is enough to make to switch to ts, want to make a PR for this?

Thanks for taking your time the PR but typescript here is not really needed so closing this. Hope you understand.

@h3nill h3nill closed this Aug 8, 2023
@carlocorradini
Copy link
Contributor Author

@henil Remove utils completely?
I agree that rumqtt is mature enough and this "verifier" can be considered unneeded:)

@h3nill
Copy link

h3nill commented Aug 8, 2023

yeah we where just thinking, but we let it be incase its needed to quickly debug protocol level things.

@carlocorradini
Copy link
Contributor Author

@henil Since we will add some configuration files for markdownlint, cspell, etc... do you want them in the root or in utils directory (I think it is better to leave them in the root since they are not utilities)

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