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: add ts types #70

Merged
merged 14 commits into from
Dec 11, 2020
Merged

feat: add ts types #70

merged 14 commits into from
Dec 11, 2020

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Dec 11, 2020

That ^

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
Copy link
Member Author

hacdias commented Dec 11, 2020

@hugomrdias all files seem to be exporting all constants and codecs in types.ts which would increase the bundle size way too much. Do you have any recommendation on how to avoid that? I understand it's related to the @typedef declaration. I could just import them inline, but that would be quite ugly.

One more question: are the types automatically generated by the release command? Didn't see any information.

Copy link
Member

@hugomrdias hugomrdias left a comment

Choose a reason for hiding this comment

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

left some comments everything else looks good

package.json Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/int-table.js Outdated Show resolved Hide resolved
tools/update-table.js Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
@hugomrdias
Copy link
Member

@hugomrdias all files seem to be exporting all constants and codecs in types.ts which would increase the bundle size way too much. Do you have any recommendation on how to avoid that? I understand it's related to the @typedef declaration. I could just import them inline, but that would be quite ugly.

i dont understand what you mean, are you talking about the .d.ts ? bundle size is not affected by this.

One more question: are the types automatically generated by the release command? Didn't see any information.

yes plus docs based on the types, aegir ts -p docs or aegir docs to check the docs.

@hacdias
Copy link
Member Author

hacdias commented Dec 11, 2020

@hugomrdias just updated according to your comments. However, the generation of the docs fail:

Screen Shot 2020-12-11 at 16 46 31

Looking at constants.d.ts, I don't think this is what we want. Before it would just say CodecNumber instead of listing all the numbers.

Screen Shot 2020-12-11 at 16 46 38

And this causes the error:

Screen Shot 2020-12-11 at 16 46 53

@hugomrdias
Copy link
Member

humm it should work check this https://multiformats.github.io/js-multihash/index.html

@hugomrdias
Copy link
Member

hugomrdias commented Dec 11, 2020

ah got it, define the Record in typescript like this https://github.com/multiformats/js-multihash/blob/master/src/types.ts

@hacdias
Copy link
Member Author

hacdias commented Dec 11, 2020

@hugomrdias unfortunately, the same error persists

@hacdias
Copy link
Member Author

hacdias commented Dec 11, 2020

Screen Shot 2020-12-11 at 17 05 44

It keeps generating these like that.

*/

/**
* @type {Record<CodecName,CodecNumber>}
Copy link
Member

Choose a reason for hiding this comment

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

this will cause the error you are seeing, use the types in types.ts instead

Copy link
Member Author

@hacdias hacdias Dec 11, 2020

Choose a reason for hiding this comment

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

If I use types.ts, then it will complain it's a recursive dependency, because types.ts gets the types from base-table. I'm separating them now. I'll need to generate two files this way anyways.

Copy link
Member Author

@hacdias hacdias Dec 11, 2020

Choose a reason for hiding this comment

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

The commit I just pushed works! not pushed...

Copy link
Member

Choose a reason for hiding this comment

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

ahhh right thats why i just ignore this type in multihash https://github.com/multiformats/js-multihash/blob/master/src/constants.js#L16 😱

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed

@hacdias
Copy link
Member Author

hacdias commented Dec 11, 2020

Screen Shot 2020-12-11 at 17 31 54

We should consider not exporting the constants on the main package. It clutters a lot the docs. We should do like in js-multihash and simply export that as a different variable. Will mention that in a differnt issue.

Copy link
Member

@hugomrdias hugomrdias left a comment

Choose a reason for hiding this comment

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

LGTM but i would call types.ts -> generated-types.ts and add a comment at the top saying "DO NOT CHANGE THIS FILE ITS AUTOGENERATED by blal blah"

@hacdias hacdias merged commit 25cf33f into master Dec 11, 2020
@hacdias hacdias deleted the types branch December 11, 2020 16:49
hacdias added a commit that referenced this pull request Dec 11, 2020
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
hacdias added a commit that referenced this pull request Dec 11, 2020
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