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

Support TypeScript #630

Closed
wants to merge 1 commit into from
Closed

Support TypeScript #630

wants to merge 1 commit into from

Conversation

MrSpoocy
Copy link

@MrSpoocy MrSpoocy commented May 13, 2022

These changes, allow to use TypeScript from now on. I use this package very often but write code exclusively with TypeScript. My goal with this pull request, is to motivate myself and others to switch this package completely to TypeScript and that @types is no longer needed.

It also allows to use mixed *.js and *.ts at the same time! But when everything is switched to TypeScript, you can use "build:types" to have the *.d.ts files generated.

If this pull request is allowed, I will immediately start rewriting single parts of the code to TypeScript. It should also not be old to complex, since no generics etc are needed.

related #598

@broofa
Copy link
Member

broofa commented May 13, 2022

Hi @MrSpoocy, thanks for the PR.

At the risk of being blunt, I don't see us taking this PR at this time, but I'd like @ctavan to weigh in with his thoughts. There are two issues that come to mind ...

  1. This isn't complete. This is just putting the tooling for compiling *.ts files in place. If/when the TS port happens, I think we'd want to do that on a feature branch so we can review and test the final, fully-ported result before merging to main.
  2. ... however there's a bigger issue at play here, which is the impact this has on how this project is maintained. Switching to TS changes the knowledge and toolchain required by all present and future maintainers.

That latter point has been the sticking point for the switch to TS for a while now. Neither @ctavan nor I has had the time to really "own" this work. And, barring finding someone new with significant TS expertise willing to join the UUIDJS org, we just haven't reached a place where we're comfortable pushing this initiative.

@MrSpoocy
Copy link
Author

I remember the first time I dealt with TS. The fear was great, especially because it was from Microsoft xD. But after a very short time, I realized that TS is not complicated. Rather the opposite, it makes it easier to write javascript. I'm sure that once @ctavan sees the first files in TS, he'll understand very quickly that it's just javascript (except for generic and overloads). I could make some files and @ctavan can have a look at them.

@MrSpoocy
Copy link
Author

MrSpoocy commented May 14, 2022

Ok, I also see that it makes more sense to put it in version 9.*. I have taken the time to rewrite the files. There are only 2 errors (and certainly a few optimizations). But the basic structure would stand!

Also ESLint (or prettier/prettier) was actually only because of too long function name. But I'm not sure if that's right, I think he counts the types (function md5ii(a: number, b: number, c: number, d: number, x: number, s: number, t: number): number {).

I also think that you can drop IE 11 with version 9.
Doesn't it make sense to work with 2 branches and thus release on npm both version 8 and 9?
I would also add UUIDv6 (https://uuid.ramsey.dev/en/stable/nonstandard/version6.html)

image

image

@broofa
Copy link
Member

broofa commented May 14, 2022

I would also add UUIDv6 (https://uuid.ramsey.dev/en/stable/nonstandard/version6.html)

FWIW, I expect https://uuid6.github.io/uuid6-ietf-draft/ will make it into this lib at some point.

@ctavan
Copy link
Member

ctavan commented May 15, 2022

So I‘m generally not opposed to porting this lib to TS since it seems like TS has become the de facto standard across the industry.

That said, what @broofa wrote is true: I‘ve personally had very little exposure to TS and since switching jobs around 1 year ago I have close to zero professional exposure to the JS ecosystem anymore (except for maintaining this library in my spare time).

I.e. translating this lib to TS would require some commitment of either an existing or a new maintainer in this org to support TS-related aspects in the foreseeable future.

@LinusU @TrySound what are your thoughts on this story?

@LinusU
Copy link
Member

LinusU commented May 29, 2022

I work almost exclusively with TypeScript and are very confident to maintain a TS port. With that said, I'm not sure if there is too much to be gained by switching the entire source code over to TS.

The public API is very limited, and seldom changes, so I think that just adding a index.d.ts file would provide typings without having to add another build step.

@ctavan
Copy link
Member

ctavan commented May 29, 2022

Historically I have always argued with the typescript docs, which pretty clearly used to recommend to only include types with packages that are authored in typescript.

This no longer seems so clear from the current wording in https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html so I think I would be fine with including the types in this package.

I also think that this should solve the concern of typescript users without the need of significantly changing the maintenance burden of this repo.

@LinusU the typescript docs mention that types can also be generated from JS files. Do you have experience with that? Is this something we could try?

@LinusU
Copy link
Member

LinusU commented Aug 1, 2022

the typescript docs mention that types can also be generated from JS files. Do you have experience with that? Is this something we could try?

I have some experience with that, and my experience is that you quickly run into types that are hard to express using only the jsdoc notations. However, this have been improved in later versions of TypeScript, and our types are so simple so I don't think that it will actually be a problem here.

If there is interest I could whip up a PR with how it could look?

@broofa
Copy link
Member

broofa commented Aug 1, 2022

If there is interest I could whip up a PR with how it could look?

That would be awesome.

BTW, it looks like VSCode can do type-checking based on JSDoc comments. We should probably enable that.

@ctavan
Copy link
Member

ctavan commented Aug 5, 2022

@LinusU do you think you can give this a try? We could still include that in the v9 release then :).

@MrSpoocy
Copy link
Author

MrSpoocy commented Aug 5, 2022

If you say yes, I will make a pull request with ~90% rewritten in TypeScript. On some types I'm not sure to handle it. But I'm sure other ppl can help to resolve the problem ;)

Maybe create a v9 branch that we not run into a conflict. It also helps to add some feature in the non-typescript version if needed.

@ctavan
Copy link
Member

ctavan commented Aug 5, 2022

Oh, I didn’t mean a typescript rewrite. I still see no benefit in that.

My request was about trying to generate the typescript types from the JS source and shipping them with this package.

@broofa
Copy link
Member

broofa commented Oct 12, 2023

Closing in preference of #654. (But very-belated thanks @MrSpoocy for taking a stab at this. TS has just been a bit of an albatross task for this project. 😕 )

@broofa broofa closed this Oct 12, 2023
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.

4 participants