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

feat: convert codebase to typescript #251

Merged
merged 16 commits into from
Dec 20, 2023
Merged

feat: convert codebase to typescript #251

merged 16 commits into from
Dec 20, 2023

Conversation

wemeetagain
Copy link
Contributor

@wemeetagain wemeetagain commented Mar 31, 2023

Fixes #249

While upgrading lodestar to typescript 5.0.x, I noticed a small issue with the autogenerated types in this library.
I figure that we can avoid the issue of autogenerated types entirely by just using typescript directly. 🤷 🚀

  • update aegir to get latest linter rules
  • minimal changes to vendored code
    • eslint-disable files, add inferred types as jsdocs, move vendor directory under src
  • convert all .js files to .ts
  • apply all jsdoc type annotations as typescript type annotations
  • fix all resulting linter and type issues
  • Special attention given to keep any helpful jsdocs

@rvagg
Copy link
Member

rvagg commented Apr 3, 2023

See #249

I appreciate the work put into this but a discussion before major work may have been a good idea.

What were the issues you were encountering, it would be good to be able to address those because this same pattern is used across many JavaScript libraries, most of which are unlikely to be converted.

@p-shahi p-shahi mentioned this pull request May 11, 2023
@wemeetagain
Copy link
Contributor Author

Did one more review of this, looks ready to me.

@p-shahi
Copy link

p-shahi commented May 30, 2023

@rvagg would like for you to give a cursory review before this gets merged. Thank you!

@rvagg
Copy link
Member

rvagg commented May 31, 2023

@p-shahi I will, it's just quite a context switch for me to review all of this. I'd also like to get @Gozala's input on this process as he was making some interesting points recently about the utilisation of JS+TSdoc (like we do here) in dag house work, and he's responsible for a lot of the recent major changes in this codebase.

@Gozala
Copy link
Contributor

Gozala commented Jun 1, 2023

@p-shahi I will, it's just quite a context switch for me to review all of this. I'd also like to get @Gozala's input on this process as he was making some interesting points recently about the utilisation of JS+TSdoc (like we do here) in dag house work, and he's responsible for a lot of the recent major changes in this codebase.

This is a huge diff which at a glance looks right, that said I'm unable to afford doing an in depth review. I do want to flag however that:

  1. It is not clear what were the issue with generated types that motivated these changes.
  2. It is not obvious that this change fixes those issues and that generated typedefs don't exhibit the same issues.

Given the above uncertainty this PR feels like "Let's typescript instead", on which I provided a more in depth feedback in #249 (comment)

@wemeetagain
Copy link
Contributor Author

this PR feels like "Let's typescript instead"

This was definitely was more of the motivation, with the issue being an excuse to justify the PR 😅
Unfortunately, I didn't really even consider interacting with (documenting, diagnosing, etc) the issue in particular. Sorry about that ☹️

My perspective is definitely biased, I'm coming from the perspective of maintaining a lot of typescript libraries. I guess I'm more sensitive to typescript typing issues (which anecdotally, I tend to see more often in javascript libraries that add types in other ways).

@BigLep
Copy link

BigLep commented Jun 7, 2023

Let's discuss the merits of this PR in the issue since that has more of the conversation: #249

examples/block-interface.ts Outdated Show resolved Hide resolved
src/block.ts Outdated Show resolved Hide resolved
src/bytes.ts Outdated Show resolved Hide resolved
src/block.ts Show resolved Hide resolved
src/cid.ts Show resolved Hide resolved
src/cid.ts Outdated Show resolved Hide resolved
src/cid.ts Outdated Show resolved Hide resolved
src/cid.ts Outdated Show resolved Hide resolved
src/cid.ts Outdated Show resolved Hide resolved
src/cid.ts Outdated Show resolved Hide resolved
src/cid.ts Show resolved Hide resolved
@rvagg
Copy link
Member

rvagg commented Jun 27, 2023

Well, I'll say that at least I don't hate it, the typedefs in jsdocs get us most of the way here anyway; is it possible to see a diff of the generated typedefs that users consume from this vs what we're exporting now? That'd be quite instructive.

Further discussion regarding whether this massive change really has enough merit vs the downsides can continue in #249. My review and attention here doesn't necessarily imply a ✅ on that front.

@SgtPooki
Copy link

SgtPooki commented Aug 2, 2023

Well, I'll say that at least I don't hate it, the typedefs in jsdocs get us most of the way here anyway; is it possible to see a diff of the generated typedefs that users consume from this vs what we're exporting now? That'd be quite instructive.

git checkout master
mkdir diff-types1

ls vendor/**/*.d.ts | xargs -tn1 -I% sh -c 'cp % diff-types1/'
ls dist/**/*.d.ts | xargs -tn1 -I% sh -c 'cp % diff-types1/'

created commit : 4a8470da50f1b696075eeedb306de464bca5b23c

### Next
git checkout feat/typescript
mkdir diff-types1

# ls vendor/**/*.d.ts | xargs -tn1 -I% sh -c 'cp % diff-types1/' # it's empty
ls dist/**/*.d.ts | xargs -tn1 -I% sh -c 'cp % diff-types1/'

git add diff-types1
git commit -m "chore: add typescript pr types" # 6c42e1b

### Merge.
git checkout ts-diff-types
git cherry-pick --strategy-option=theirs 6c42e1bedb7090f080ad31eaf41b422a9637f867

git push -u mine ts-diff-types

@rvagg @wemeetagain
see the diff at 0ead76d

@BigLep
Copy link

BigLep commented Sep 26, 2023

Discussion on next steps: #249 (comment)

@maschad
Copy link

maschad commented Oct 3, 2023

Once @wemeetagain resolves the conflicts and pushes this to a ready state and gets approval, I will test this with js-libp2p stack before we merge

@maschad
Copy link

maschad commented Oct 17, 2023

I've ran this with our js-libp2p test suite successfully so this should be good to merge.

@BigLep
Copy link

BigLep commented Dec 1, 2023

@wemeetagain : what's the status on this? Are we going to be able to get this merged this year?

@wemeetagain
Copy link
Contributor Author

Just freshened this up. I believe I've now resolved all outstanding review comments.
Looking for one last review from @achingbrain

🤞 for a 🎅 🎄 miracle

Copy link

@maschad maschad left a comment

Choose a reason for hiding this comment

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

I've tested out these changes in libp2p/js-libp2p#2153 and the test suite ran successfully

@achingbrain achingbrain merged commit 3557219 into master Dec 20, 2023
18 checks passed
@achingbrain achingbrain deleted the feat/typescript branch December 20, 2023 08:06
achingbrain added a commit that referenced this pull request Dec 20, 2023
Fixes #249

- update aegir to get latest linter rules
- minimal changes to vendored code
  - eslint-disable files, add inferred types as jsdocs, move vendor directory under src
- convert all .js files to .ts
- apply all jsdoc type annotations as typescript type annotations
- fix all resulting linter and type issues
- Special attention given to keep any helpful jsdocs

BREAKING CHANGE: this module is now TypeScript - the API has not changed but releasing as a major for saftey

---------

Co-authored-by: Alex Potsides <alex@achingbrain.net>
github-actions bot pushed a commit that referenced this pull request Dec 20, 2023
## [13.0.0](v12.1.3...v13.0.0) (2023-12-20)

### ⚠ BREAKING CHANGES

* this module is now TypeScript - the API has not changed but releasing as a major for saftey

### Features

* convert codebase to typescript ([#251](#251)) ([fcee86e](fcee86e)), closes [#249](#249)

### Trivial Changes

* **deps:** bump actions/setup-node from 3.8.2 to 4.0.0 ([139b3c2](139b3c2))
github-actions bot pushed a commit that referenced this pull request Dec 20, 2023
## [13.0.0](v12.1.3...v13.0.0) (2023-12-20)

### ⚠ BREAKING CHANGES

* this module is now TypeScript - the API has not changed but releasing as a major for saftey

### Features

* convert codebase to typescript ([#251](#251)) ([fcee86e](fcee86e)), closes [#249](#249)

### Trivial Changes

* **deps:** bump actions/setup-node from 3.8.2 to 4.0.0 ([139b3c2](139b3c2))
achingbrain added a commit that referenced this pull request Dec 20, 2023
Fixes #249

- update aegir to get latest linter rules
- minimal changes to vendored code
  - eslint-disable files, add inferred types as jsdocs, move vendor directory under src
- convert all .js files to .ts
- apply all jsdoc type annotations as typescript type annotations
- fix all resulting linter and type issues
- Special attention given to keep any helpful jsdocs

BREAKING CHANGE: this module is now TypeScript - the API has not changed but releasing as a major for saftey

---------

Co-authored-by: Alex Potsides <alex@achingbrain.net>
github-actions bot pushed a commit that referenced this pull request Dec 20, 2023
## [13.0.0](v12.1.3...v13.0.0) (2023-12-20)

### ⚠ BREAKING CHANGES

* this module is now TypeScript - the API has not changed but releasing as a major for saftey

### Features

* convert codebase to typescript ([#251](#251)) ([56bbb96](56bbb96)), closes [#249](#249)

### Trivial Changes

* **deps:** bump actions/setup-node from 3.8.2 to 4.0.0 ([139b3c2](139b3c2))
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.

When TypeScript?
8 participants