Skip to content
This repository has been archived by the owner on Dec 6, 2022. It is now read-only.

feat: add typedef generation #56

Merged
merged 6 commits into from
Oct 20, 2020
Merged

feat: add typedef generation #56

merged 6 commits into from
Oct 20, 2020

Conversation

Gozala
Copy link
Contributor

@Gozala Gozala commented Oct 14, 2020

Main goal of this pull request is to start generating typescript typedefs from the jsdocs so that dependent libraries can take advantage of them out of the box. Mainly I'd like this to get better types in js-ipfs (see ipfs/js-ipfs#3281)

This pull request does a little bit more though:

.eslintrc Show resolved Hide resolved
runs-on: ubuntu-latest
strategy:
matrix:
node-version: [12.x]
Copy link
Member

Choose a reason for hiding this comment

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

any reason to stick with 12.x, can we bump to 14.x to future proof this a bit more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I honestly wish there was a way to stay whatever the current stable version is, but I can't find a way to do that. Chose 12.x because it's actually newer that 10.x used here and current LTS. Happy to put whatever makes most sense.

Please note that this just runs the typescript not any of the code here.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, so I think 14.x is going to be the best bet here, it's got a much longer life than 12.x ahead of it and is 2 weeks away from being LTS so we may as well start using it in places like this. up to you though.

The labels thing has been an ongoing discussion for a while for setup-node with folks wanting it to copy the nvm style so we could do the same things that we could do with travis, alas no serious movement: actions/setup-node#26

static isBlock (other) { // eslint-disable-line no-unused-vars
// implemented by class-is module
static isBlock (other) {
return Boolean(other && other[blockSymbol])
Copy link
Member

Choose a reason for hiding this comment

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

maybe just return other && other[blockSymbol] === true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rvagg I'm afraid TS will complain because null && v is null and not a boolean.

Copy link
Member

Choose a reason for hiding this comment

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

urgh, that's kind of gross, return !!other && other[blockSymbol] === true?

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

sgtm, and +1 to removal of class-is to simplify

@ipld ipld deleted a comment from codecov bot Oct 14, 2020
@codecov
Copy link

codecov bot commented Oct 14, 2020

Codecov Report

Merging #56 into master will increase coverage by 10.83%.
The diff coverage is 95.83%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #56       +/-   ##
===========================================
+ Coverage   85.71%   96.55%   +10.83%     
===========================================
  Files           1        1               
  Lines          14       29       +15     
===========================================
+ Hits           12       28       +16     
+ Misses          2        1        -1     
Impacted Files Coverage Δ
src/index.js 96.55% <95.83%> (+10.83%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 787cff3...ddd1843. Read the comment docs.

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