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

Adding Types #496

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Adding Types #496

wants to merge 1 commit into from

Conversation

leonardobsjr
Copy link

Adding TS types.

@leonardobsjr leonardobsjr marked this pull request as ready for review February 22, 2024 14:37
@ExplodingCabbage
Copy link
Collaborator

ExplodingCabbage commented Mar 12, 2024

Hmm. So I see that this adds declarations of the functions that jsdiff exports but all parameters and return types are just set to any, which makes them kinda non-useful. I guess I could use those as a start point and manually modify them. But it'd be nice to have useful types generated automatically.

I'm not too sure what the best way to do that is, though. Probably I should port the whole library to TypeScript, adding types to function signatures as I go, and change its build process to compile from TypeScript?

@ehoogeveen-medweb
Copy link

Another way would be to add jsdoc comments for the exported functions - IIRC those get picked up by tsc and used in the generated declaration files.

@ExplodingCabbage
Copy link
Collaborator

Ah, nice! That does indeed seem to work and I guess it'll do the trick.

@leonardobsjr
Copy link
Author

Yeah, I understand that this is not ideal @ExplodingCabbage - This is just a way of making sure that every jsdiff version has up-to-date method information to be used in typescript. The issue that motivated me was not being able to use a method that existed on the latest version of jsdiff because the type was not updated on https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/diff.

That being said, @ehoogeveen-medweb suggestion is far better than porting the whole stuff lol

@ehoogeveen-medweb
Copy link

Well, I don't know about better as I do like TypeScript a lot, but it's certainly a lot less work ;)

@ExplodingCabbage
Copy link
Collaborator

Okay, coming back to this after a break...

I think porting to TypeScript is probably actually no more work than adding JSDoc comments would be. One problem with adding JSDoc comments is that some TypeScript concepts don't quite have clean analogues in JSDoc. For instance, the diffChars / diffWords / etc functions have a bunch of common options, so it makes sense to define a BaseDiffOptions interface and then have DiffCharsOptions extends BaseDiffOptions, DiffWordsOptions extends BaseDiffOptions, etc, but JSDoc doesn't seem to cleanly support this. Defining a shared type that's used in multiple places and then importing it from other files seems to be a bit weird/tricky/verbose in JSDoc?

I'm gonna have a stab at actually porting everything to TypeScript and using Babel's preset-typescript to transpile to JavaScript (as seems to be the recommendation at https://www.typescriptlang.org/docs/handbook/babel-with-typescript.html if you need a build pipeline anyway); I'm hoping that besides actually sprinkling TypeScript types into all the function definitions, there's not gonna be much work involved in this beyond adding a few lines to the Babel config. Will see if that hope is proved accurate...

@ehoogeveen-medweb
Copy link

By the way, one thing that might help in this process is https://arethetypeswrong.github.io or https://www.npmjs.com/package/@arethetypeswrong/cli (right now there are no types, but once there are, getting CJS and ESM to both work can be tricky).

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