-
-
Notifications
You must be signed in to change notification settings - Fork 903
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 TypeScript typings #654
Conversation
I think this looks really good, although – given my lack of TS experience – I can't judge the quality of the types. Questions that come to my mind:
In any case, thanks a lot for putting this together and moving this long-requested feature forward! |
@@ -29,7 +29,11 @@ | |||
"import": "./dist/esm-browser/index.js", | |||
"require": "./dist/commonjs-browser/index.js" | |||
}, | |||
"default": "./dist/esm-browser/index.js" | |||
"default": "./dist/esm-browser/index.js", | |||
"types": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
types
should be first for some reason: https://www.typescriptlang.org/docs/handbook/esm-node.html#packagejson-exports-imports-and-self-referencing
"default": "./dist/esm-browser/index.js" | ||
"default": "./dist/esm-browser/index.js", | ||
"types": { | ||
"module": "./types.d.mts", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"module": "./types.d.mts", | |
"import": "./types.d.mts", |
I've not seen module
as an export condition before - could you point to its origin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is a webpack thing, possibly adopted by other packaging tools as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, cool. These are types tho, and the TS docs only point to node (where the spec comes from), and they don't have a module
condition listed there: https://nodejs.org/api/packages.html#conditional-exports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took it from DefinitelyTyped/DefinitelyTyped#57194, but it was changed to import
later which is correct. Good catch! 👍
* uuidValidateV4(v1Uuid); // ⇨ false | ||
* ``` | ||
*/ | ||
export function validate(str: string): boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to support #606:
export function validate(str: string): boolean | |
export function validate(str: any): str is string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nominal types would have made this way cleaner 😛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah for sure!
Would it make sense to use JSDoc to document the actual source, then have |
Left a note to myself to adress this 👍
I'm actually not sure why it was made that way 🤔 It seems like it was introduced in DefinitelyTyped/DefinitelyTyped#16264. @felipeochoa sorry for a ping for something you made 5 years ago 🙈, but do you happen to remember why use used: export type v1String = (options?: V1Options) => string;
export type v1Buffer = <T extends OutputBuffer>(options: V1Options | null | undefined, buffer: T, offset?: number) => T;
export type v1 = v1String & v1Buffer; instead of using an overloaded function? export function v1(options?: V1Options | null): string
export function v1<T extends ArrayLike<number>>(options: V1Options | null | undefined, buffer: T, offset?: number): T
Awesome 🙌 |
This was actually my first approach, but I ran into a number of small issues. The biggest hurdle was probably that that would generate one If we go that route we could also run the TypeScript compiler to type check our JavaScript codebase with |
types.d.ts
Outdated
* uuidVersion('6ec0bd7f-11c0-43da-975e-2a8ad9ebae0b'); // ⇨ 4 | ||
* ``` | ||
*/ | ||
export function version(str: string): boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export function version(str: string): boolean | |
export function version(str: string): 1 | 3 | 4 | 5 |
or number
? deffo not boolean
, tho 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hehe, not sure how it ended up as boolean
😅
It can return 0
also, but yeah number union seems like a great return value, thanks! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... worth adding a unit-test for types of some sort? Jest + tsc
a few sample scripts to verify it does / doesn't throw the expected type warnings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend using tsd
or something. In the jest repo we use https://github.com/jest-community/jest-runner-tsd which works great (if you already use jest), but tsd is great for type tests
I made some progress on keeping the docs in sync. The most things are already supported by ts-readme-generator, and I've added some small missing things now. There is still some things left though, the main thing being how to handle overloaded functions 😅 But we only need a solution that's good enough for our specific needs, so that makes it a bit easier. I'm thinking now that it could compare if the overload only appends new arguments, and then merge them into just one heading. Should be quite easy! I've put a todo list in the PR description to track what's left... |
Might want to add an item for removing |
@broofa Ahh, I see now that |
Note to self: change return value to ref: DefinitelyTyped/DefinitelyTyped#64798, microsoft/TypeScript-DOM-lib-generator#1432 |
Is this a Template Literal Type? Seems like an appropriate change. |
Yes it is! Yeah, I think it's a good change, since it would allow people to be stricter with functions that only accept uuids, and not any string 🚀 Also, sorry for falling behind on this, unfortunately I have a bit limited time at the moment, but I really want to push thru with this! |
No need to apologize. We're all in the same boat. ⛵️ |
FWIW, the more I think about this issue (adding type declarations to this project), the more I'm of the opinion that we should just port everything to TS, and let Having this code be in TS would've given me more confidence that we're not introducing regressions. It would also force us to be more strict about what types are supported (e.g. "What type, exactly, are binary UUIDs?"). And, of course, it'd avoid having to manually update the *.d.ts file here. @pmccarren What's your comfort level with Typescript? Would porting this codebase to TS help or hinder your interest in helping to maintain this project? |
@broofa I'm all in for typescript, and I believe it a worthwhile investment. I too feel it would build confidence working throughout the repo As for comfort level, I've worked extensively with typed languages and while bit newer to the TS scene specifically, I've began adopting it exclusively for all new JS projects. v10 focused on RFC9562, v11 TS + deploy tooling? |
Yup. TS and deploy tooling are out of scope for v10.0.0 (which I'd like to release ASAP). FWIW, I expect deploy tooling to be independent of semver releases. (although I expect we'll want to do actual testing of the deploy pipeline, that can/should happen with prerelease tags that stay off the main release timeline.) As for TS, switching to TS in and of itself wouldn't necessitate a major release since the API wouldn't necessarily change. That said, it wouldn't surprise me if one of the side effects was tighter type restrictions in the API (e.g. ... but whatevs. Cross those bridges post 10.0.0. |
Closing. This PR seems to have eddied out (for now), and I think the path forward will be to port the @LinusU Thanks for the effort. Sorry we didn't get this merged. So... uh... 'feel like porting some JS to TS?? 😉 |
I've made this typings by copying in everything from the readme and then formatting it properly. Opening as a draft for now as I would like to investigate some way to make sure that the typings and readme are always in sync. Potentially ts-readme-generator or a similar script. Otherwise I fear that the typings and readme file will drift away from each other 😅
ts-readme-generator
@throws
@example
@note
V1Options
/V4Options
isn't expanded@example
(and others) on exported constantstsd
runmd
dependency andREADME_js.md
file