Skip to content
This repository has been archived by the owner on Feb 12, 2024. It is now read-only.

feat: type check & generate defs from jsdoc #3281

Merged
merged 79 commits into from
Oct 21, 2020
Merged

feat: type check & generate defs from jsdoc #3281

merged 79 commits into from
Oct 21, 2020

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented Sep 11, 2020

This pull request is an attempt to do following:

  1. Add missing type info (via jsdoc) and fix existing one so that tsc --noEmit can succeed without an error.
  2. Fix all the issues that tsc pointed out once it got enabled.
  3. Add a build script that generates typedefs & typedef maps in dist directory & corresponding settings in package.json so that when package installed from npm no config will be required to get all the typings.

Unfortunately it is intertwined with some of the eslint work that I have started, in order to get rid of all the disabled warning about jsdoc comments. I was hoping we'd do ipfs/aegir#636, but if aren't doing that we'd need to port some changes into aegir and only after we'll be able to remove those eslint disables (and possibly we'd need to add more of those too).

Note that currently generated typedefs are not a good quality yet, mostly because big parts of code base aren't typed and there is quite a bit dependence on inference which I think could be improved. However I think it would be good to start here and improve by tweaking things in the repo.

@achingbrain I would really like to get this inn as soon as possible because it touches almost every single file in the repo, which would make it difficult to keep it up to date. If you're able to do a preliminary review it would be really great. I also really don't want to block on aegir, I would much rather get typedef generation and validation working and then improved it by migrate some of those pieces to the aegir.

Dependencies:

@achingbrain
Copy link
Member

This is a massive PR, it's going to take some time to go through all the changes here.

Is this really the smallest changeset that accomplishes this task?

@Gozala
Copy link
Contributor Author

Gozala commented Sep 14, 2020

Is this really the smallest changeset that accomplishes this task?

There are few things that I could do to make it somewhat smaller:

  1. Remove all the comments that do not contain type info. (Right now big chunk of documentation from .md files had being added as part of jsdoc commens with intent to generate docs later).
  2. I could break it a part into smaller patches, but only after landing a last one we would be able to generate docs and ts will succeed in checking / generating. (I'm less eager to do that, because chances are once we land all the pieces things will no longer typecheck as things could have gone out of sync).
  3. Detangle eslint related changes from here. Which is something I was going to do anyway. That said it's not going to change size of this pull request much.

Please let me know if you want me to do 1. or the 2. I'm also happy to jump on the call if you want a walkthrough of the changes to make reviewing easier.

@Gozala Gozala requested a review from jacobheun October 5, 2020 19:45
@Gozala
Copy link
Contributor Author

Gozala commented Oct 5, 2020

@jacobheun you've suggested taking some things off of @achingbrain's plate. I incorporated repo overhaul changes from @achingbrain so it should be ready.

@achingbrain
Copy link
Member

Thanks for tidying this up with all the churn in master.

CI is very red, could you make sure the tests pass please?

@achingbrain achingbrain merged commit bbcaf34 into master Oct 21, 2020
@achingbrain achingbrain deleted the type-gen branch October 21, 2020 16:33
SgtPooki referenced this pull request in ipfs/js-kubo-rpc-client Aug 18, 2022
1. Add missing type info (via jsdoc) and fix existing one so that `tsc --noEmit` can succeed without an error.
2. Fix all the issues that `tsc` pointed out once it got enabled.
3. Add a npm script that generates typedefs & typedef maps in `dist` directory & corresponding settings in `package.json` so that when package installed from npm no config will be required to get all the typings.

Co-authored-by: Xmader <[email protected]>
Co-authored-by: Alex Potsides <[email protected]>
Co-authored-by: achingbrain <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants