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: ts definitions #4

Closed
wants to merge 4 commits into from
Closed

Conversation

AuHau
Copy link

@AuHau AuHau commented Sep 9, 2020

Since you have good JSDoc types it was easy-peasy to generate these. You can regenerate that easily with running tsc -d --emitDeclarationOnly --allowJs *.js. tsc is already bundled in aegir.

Maybe it is worth considering to have this command in prePublish hook and generate those upon release? And not polluting the repo?

Closes #3

@achingbrain
Copy link
Owner

Thanks for submitting this. Doing it as a prePublish hook sounds sensible, though it would probably need to be prepublishOnly to prevent it being run after install.

Could you please update the PR to do that?

@AuHau
Copy link
Author

AuHau commented Sep 9, 2020

Done, although currently, the generation fails thanks to Gozala/web-encoding#7

So I guess the NPM publish would fail thanks to that as well, so will have to resolve that issue first.

@AuHau
Copy link
Author

AuHau commented Sep 9, 2020

Fix in the web-encoding is on the way :-)

Gozala/web-encoding#8

achingbrain added a commit to ipfs/aegir that referenced this pull request Sep 11, 2020
Adds a `generate-types` command to generate `*.d.ts` files.

Largely inspired by @AuHau's work on achingbrain/uint8arrays#4
@Gozala
Copy link
Contributor

Gozala commented Sep 14, 2020

One thing you may want to consider is to generate types into separate dir e.g. dist like it's done here https://github.com/ipfs/js-ipfs/pull/3281/files#diff-35d110cab7df7eeb28025afa7b96ed28. typesVersions field tells TS to resolve imports from dist instead of root of the package.

@AuHau
Copy link
Author

AuHau commented Sep 15, 2020

I would be a bit against to put it into dist as I believe the unspoken convention is to put browser build files there (at least in aegir context). Also if they are put into a separate folder then it is usually named types.

I am open to change this and will leave that up to @achingbrain but since this package follows a bit different structure than the standard "put everything into /src" I have left it like this as well...

Hmm did not know about typesVersions. Isn't for this usually used types property in package.json?

@Gozala
Copy link
Contributor

Gozala commented Sep 15, 2020

would be a bit against to put it into dist as I believe the unspoken convention is to put browser build files there (at least in aegir context).

My interpretation of unspoken rule was build artifacts go to dist 😅

Also if they are put into a separate folder then it is usually named types.

I think types usually host typedefs for third party package defs that were not found in @types/ namespace.

I am open to change this and will leave that up to @achingbrain but since this package follows a bit different structure than the standard "put everything into /src" I have left it like this as well...

I don't hold strong preferences either way, just want to consolidate on the similar pattern across different packages in the ecosystem. For IPFS I went with placing them in dist ipfs/js-ipfs#3281, but don't mind using some other dir instead.

Edit: Only reason I generally don't like generating *.d.ts files next to source js files is because in some cases you might have *.d.ts files and it becomes nuisance to remove generated ones and keep hand written ones. It does not matter in this context, but if we decide to converge onto same approach across the board it's worth considering.

Hmm did not know about typesVersions. Isn't for this usually used types property in package.json ?

I think it is fairly new, or at least I have discovered it pretty recently myself. As of types property it can only point to a single file, which could have multiple define module statements to host multiple modules, but I don't believe typescript has a way to generate such files. typeVersions maps to directory and there for generated typedefs could be used as is.

@AuHau
Copy link
Author

AuHau commented Sep 16, 2020

My interpretation of unspoken rule was build artifacts go to dist 😅

😅 Well, you are right I guess. Just coming from TS development background we use the following structure for general libraries and so I was a bit biased here:

├── dist - webpacked JS files
├── lib - compiled TS files into JS files
├── src - the main TS files (not released in npm package)
│   └── @types - for placing third-party package types defs
└── types - generated types def

But it is true that I can't think of some really "true" setup that is standardized for TS development. For example I saw some people replacing src folder with ts...

But if you would put it into dist would you put it directly into the root of dist or nested it into something like /dist/types? Because otherwise, it would be clashing with other build artifacts (like for example the browser files dists)

I think types usually host typedefs for third party package defs that were not found in @types/ namespace.

That is possible, but we use bit different setup 😅

I think it is fairly new, or at least I have discovered it pretty recently myself. As of types property it can only point to a single file, which could have multiple define module statements to host multiple modules, but I don't believe typescript has a way to generate such files. typeVersions maps to directory and there for generated typedefs could be used as is.

Interesting, you are right! It works :-D I just tried it on this PR... It feels still a bit weird to "abuse" this field in this way.

I still think that for general usage the types is enough because it mimics the package.json's main field. But it is true that because of it I am commonly exporting interfaces in index.ts just to get it to the root of the package.

TLDR; @achingbrain let me know what you think, I am fine with whatever 😆

@Gozala
Copy link
Contributor

Gozala commented Sep 16, 2020

But if you would put it into dist would you put it directly into the root of dist or nested it into something like /dist/types? Because otherwise, it would be clashing with other build artifacts (like for example the browser files dists)

It is a good question in this repo context, in cases where there's an src it usually ends up in dist/src. That said generated files will have a .d.ts extensions so I don't think it would really collide with other artifacts.

@achingbrain
Copy link
Owner

Since typesVersions solves the click-through thing I would just stick them in /dist as they are generated artefacts that we want to distribute.

Happy to re-evaluate if it causes problems/affront.

@AuHau
Copy link
Author

AuHau commented Sep 17, 2020

Wait, it does? I did not know that! Well if that is the case, then for sure I would go that way!

I have updated the PR then :-)

@Gozala
Copy link
Contributor

Gozala commented Oct 7, 2020

Small clarification, typesVersions does not solve click-through problem, declarationMaps do. But given that declarationMap solves that there is no compelling reason to put generated typedefs next to source files.

@AuHau
Copy link
Author

AuHau commented Nov 20, 2020

@achingbrain what about this PR? 😇

@achingbrain
Copy link
Owner

ipfs/aegir#671 has just landed in aegir which adds typedef generation support so hopefully this should become obsolete and we can handle it all through the tooling.

@wemeetagain wemeetagain mentioned this pull request Dec 12, 2020
1 task
achingbrain pushed a commit that referenced this pull request Dec 18, 2020
Minimal updates to add autogenerated typescript types via aegir.
Used this doc to help: https://github.com/ipfs/aegir/blob/master/md/ts-jsdoc.md

I tested this PR locally, linking this to a test project (including updated `web-encoding` dependency) to make sure the types all line up.

See #4 (comment)
Resolves #3
@achingbrain
Copy link
Owner

Superseded by #7

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.

Typescript definitions
3 participants