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

refactor: use type-checked JS instead of TS for quickstart examples #1215

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

Conversation

JKRhb
Copy link
Member

@JKRhb JKRhb commented Jan 12, 2024

This PR contains an initial proposal for a partial implementation of #1108, removing the (sort of redundant) TS files for the quickstart examples and instead adding type checking via a @ts-check comment to the respective JS example files. Additional type information is added via JSDoc comments.

This PR can serve as a basis for discussing if this is the direction we want to go with the other examples as well – if so, then I would open PRs for the other examples (or rather: those that should remain in the node-wot repository) as well.

@JKRhb JKRhb changed the title refactor: use type-check JS instead of TS for quickstart examples refactor: use type-checked JS instead of TS for quickstart examples Jan 12, 2024
Copy link

codecov bot commented Jan 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.81%. Comparing base (0b7b108) to head (5f123ea).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1215   +/-   ##
=======================================
  Coverage   77.81%   77.81%           
=======================================
  Files          84       84           
  Lines       17523    17523           
  Branches     1781     1781           
=======================================
  Hits        13635    13635           
  Misses       3853     3853           
  Partials       35       35           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@relu91
Copy link
Member

relu91 commented Jan 12, 2024

I'm ok exploring this, but the main reason why we had this "duplicated" code in the first time was to show people how the example would look in two different languages. Using @js-check will only make the JS code type-checked but we are going to lose the TS version of the example.

@JKRhb
Copy link
Member Author

JKRhb commented Jan 12, 2024

(I initially linked the wrong issue in the PR description, I meant to be referring to #1108, which is now fixed.)

@JKRhb
Copy link
Member Author

JKRhb commented Jan 12, 2024

the main reason why we had this "duplicated" code in the first time was to show people how the example would look in two different languages.

Hmm, I see, I wasn't aware of that. I think the current JS examples might not be that great in that respect, though, since they contain some polyfills (e.g. for enums) and lines like const core_1 = require("@node-wot/core"); which I don't find that nice. As mentioned in #1108, using JS with type-checks could be a more elegant solution than the current approach, since TypeScript is a superset of JavaScript anyway, so we would not have to maintain the pipeline of copying over the files from the examples package. But this is just a proposal that I wanted to discuss with you, if we should decide against it, that's also fine for me :)

@danielpeintner
Copy link
Member

I tried it out locally and it works nice... similar behavior in VS code as with TS files 👍

On the one hand I agree with @relu91 that having samples for both TS and JS is beneficial.
On the other hand I also dislike the way we create out of readable TS samples less readable (ugly looking?) JS samples.

Mhhh, not sure what we should do...
We definitely do not need having the same samples in both places.

I wonder what is most used TS or JS ?
How could we achieve a better split?

@JKRhb
Copy link
Member Author

JKRhb commented Feb 27, 2024

I tried it out locally and it works nice... similar behavior in VS code as with TS files 👍

On the one hand I agree with @relu91 that having samples for both TS and JS is beneficial. On the other hand I also dislike the way we create out of readable TS samples less readable (ugly looking?) JS samples.

Mhhh, not sure what we should do... We definitely do not need having the same samples in both places.

I wonder what is most used TS or JS ? How could we achieve a better split?

Hmm, yeah, with Node.js, the situation is also not ideal since JavaScript with the CommonJS syntax is not a true subset of TypeScript (for that we would need to use ES Modules, but their compatibility with the older CommonJS approach is somewhat limited). Otherwise, we could cover both languages at the same time :/

However, as every TypeScript developer should be familiar with JavaScript, I would personally say that using only JS examples should cover the largest amount of people while making the examples themselves more accessible.

@egekorkan
Copy link
Member

I think I like this approach, some thoughts:

  • I am not sure if outsiders find the ts examples anyways
  • I also did not like the funny variable names in the JS examples
  • Making sure to compile and do the sort of manual changes when I do contribution to examples is annoying.
  • User perspective*: For anything that is a quick demo, I do it JS. If I use node-wot as a dependency of something bigger, I use TS. Easier to use JS files are nicer in this case. When building something bigger, sometimes I have issues with some types and want to see an example of their usage. These examples help but so would tests. This concern would be totally invalid once we have end to end tests, that are almost examples. Those test Things and Consumers would in TS and would address my need.
  • I say user since I see myself more of a user of node-wot rather than a core developer.

@danielpeintner
Copy link
Member

danielpeintner commented Feb 28, 2024

Does it make sense to put this topic on the agenda of one of our next weekly calls?

EDIT: Maybe an according label would be also good so that we can easily find topics/issues for the calls...

@JKRhb JKRhb added Discuss in Committer Meeting Labels issues and PRs that should be discussed in our regular Thingweb Committer Meeting. examples Issues related examples in the repository labels Feb 29, 2024
@JKRhb
Copy link
Member Author

JKRhb commented Feb 29, 2024

EDIT: Maybe an according label would be also good so that we can easily find topics/issues for the calls...

I now created a label called "Discuss in Committer Meeting" for that :)

@egekorkan
Copy link
Member

Some discussions from the meeting today:

  • Ege: A simple hand-written js example, like counter, is given to the users first. They can run it easily and understand node-wot. That example (its documentation) links to TS and JS examples of other examples we have, which are used for using node-wot in more advanced cases. Those other examples are maintained with a single codebase that we can use for testing etc.
  • Jan: We need to check whether we can generate js-doc typed js examples. Like https://github.com/futurGH/ts-to-jsdoc
  • We should check with @relu91

@relu91
Copy link
Member

relu91 commented Dec 16, 2024

Working with ES2021 does not seem to generate that ugly JS anymore, what do you think?

@danielpeintner
Copy link
Member

Working with ES2021 does not seem to generate that ugly JS anymore, what do you think?

That is great! Once we move on with #1340 we can also remove the workflow described here

### Workflow
1. Run `npm run build`
2. Remove the following 3/4 lines in JS files of folder `dist/`
```
Object.defineProperty(exports, "__esModule", { value: true });
require("wot-typescript-definitions");
let WoT;
let WoTHelpers;
```
3. Copy the according JS file(s) to
- `<node-wot>/examples/scripts`
- `<node-wot>/examples/testthing`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discuss in Committer Meeting Labels issues and PRs that should be discussed in our regular Thingweb Committer Meeting. examples Issues related examples in the repository
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants