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

Added Type Definitions #5

Merged
merged 2 commits into from
Aug 4, 2021
Merged

Added Type Definitions #5

merged 2 commits into from
Aug 4, 2021

Conversation

guytepper
Copy link
Contributor

Added type definitions for formatDuration

Thanks for the small yet useful library! :)

Copy link
Owner

@ungoldman ungoldman left a comment

Choose a reason for hiding this comment

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

seems fine to me 🤷

@ungoldman
Copy link
Owner

@bcomnes I have successfully avoided getting slurped into the world of TypeScript maintenance so far. Any downsides to this? I know in the past other libraries have recommended maintaining types separately through some third party type definition aggregation library or something.

@bcomnes
Copy link
Contributor

bcomnes commented Jul 13, 2021

My only preference would be to use js-doc, unless this offers a speific-to-this-library advantage with the index.d.ts approach. That way you get the little poppy uppy docs in your ed and I think type checking.

@ungoldman
Copy link
Owner

@guytepper can you get the same mileage out of just modifying the main file to have the function definition in jsdoc?

@guytepper
Copy link
Contributor Author

guytepper commented Jul 13, 2021

Sure, but I think it might result in a more complicated flow - at least that's how it seems.

I haven't done the jsdoc-to-ts approach before so I might be wrong here - @bcomnes have you ever done that?

@bcomnes
Copy link
Contributor

bcomnes commented Jul 13, 2021

Thats converting types. If types are defined in a module as a js-doc, TS should pick those up automatically without work. .d.ts definitely has more capabilities and features, but in the case of this module, we would not be using any of those. Let me draft up an example (based on your work).

@bcomnes bcomnes mentioned this pull request Jul 13, 2021
@bcomnes
Copy link
Contributor

bcomnes commented Jul 13, 2021

@guytepper Ok, I converted it over here #6

I'm going to publish a beta version, would you be willing to test it out and let me know if it works (or doesn't work) to the level you expect?

@bcomnes
Copy link
Contributor

bcomnes commented Jul 13, 2021

@ungoldman can you give me publish rights on npm? I'm going to try this out as a beta

@ungoldman
Copy link
Owner

@bcomnes you should be added now!

@ungoldman
Copy link
Owner

lol wait!!!!

look at this: https://www.npmjs.com/package/@types/format-duration

@ungoldman
Copy link
Owner

Screen Shot 2021-07-13 at 1 43 08 PM

@ungoldman
Copy link
Owner

I knew this sounded familiar DefinitelyTyped/DefinitelyTyped#21893

@bcomnes
Copy link
Contributor

bcomnes commented Jul 13, 2021

I think thats the workaround people have to install additionally if they want types. This would make it a single package solution. I'm not opposed to doing js-doc blocs if it works for Typescripters, especially if its so simple.

@guytepper
Copy link
Contributor Author

yup, I'd prefer to not install an additional package for getting types :)

ready to report once the beta gets released!

@bcomnes
Copy link
Contributor

bcomnes commented Jul 14, 2021

Ok, I published a beta tag (1.3.2-beta.0) @guytepper can you let me know if that works for you?

@guytepper
Copy link
Contributor Author

guytepper commented Jul 15, 2021

It doesn't seem to be working, the type declarations are not found

@bcomnes
Copy link
Contributor

bcomnes commented Jul 15, 2021

K, gonna play around with it.

@bcomnes
Copy link
Contributor

bcomnes commented Jul 15, 2021

Just messed up and accidentally published on the latest channel. I wen't ahead and republished a version ahead since you can't unpublish on npm any more. Sorry for the noise, npm is unforgiving if you hit enter too early.

@bcomnes
Copy link
Contributor

bcomnes commented Jul 15, 2021

Ok, so I did a little bit of digging to finally figure this out.

So TS doesn't actually consume JSDoc strings from node_modules, because of performance or reasons. But you can author a typed module from JSDoc strings, you just need to adopt the ts toolchain and add a build step. @voxpelli wrote a great article on this method:

https://voxpelli.com/2019/10/use-type-script-3-7-to-generate/

I don't want to do that here. What I would be fine with:

  • Landing this and just calling it good. Yay for doccy poppy uppys.

or

  • Be a conservative maintainer and just declare that types are not really going to help anyone with a function this small, and that if you really want types just use automaticTypeAquisition. (that isn't to say I don't appreciate your time and effort @guytepper, I do, thank you)

@ungoldman your call.

Glad to have finally figured this question I had out about the usefulness of JSDocs though.

@guytepper
Copy link
Contributor Author

Thanks for that information.

In that case I wonder why not to go with the index.d.ts method - isn't it better than not having types at all?

I'm using this function frequently in a project and it'd be nice to have the benefits TS provides

@ungoldman
Copy link
Owner

ungoldman commented Jul 21, 2021

@bcomnes I don't feel strongly either way. if you both want to just merge and call it good I'm fine with that.

(my main issue with PRs like this is introducing TypeScript support to non-TypeScript projects means the module maintainers now have to maintain it, not the person who introduced the PR. In my case I have no interest in doing long-term TypeScript maintenance for any of my projects, especially very old ones. Since this module is tiny and hasn't changed in years, and you've both put the work in, I say go for it 🤷)

@bcomnes
Copy link
Contributor

bcomnes commented Aug 3, 2021

OK we can just land this then.

@ungoldman ungoldman merged commit 7842973 into ungoldman:master Aug 4, 2021
@ungoldman
Copy link
Owner

ungoldman commented Aug 4, 2021

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.

None yet

3 participants