Skip to content

TypeScript rewrite #591

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

Open
wants to merge 152 commits into
base: master
Choose a base branch
from
Open

TypeScript rewrite #591

wants to merge 152 commits into from

Conversation

ExplodingCabbage
Copy link
Collaborator

@ExplodingCabbage ExplodingCabbage commented Feb 28, 2025

Resolves #303
Resolves #577

See also discussion at #583.

This turned out to be a kind of hellish amount of work, but I think I've cracked it. This should eliminate the need for users to install @types/diff and also fix various issues with the types currently on DefinitelyTyped, along with making sure that henceforth there'll be no possibility of type declarations being out of sync with the underlying library. (TypeScript also caught two actual bugs in the library during the course of the rewrite, fixed separately in #601 and #602.)

@ExplodingCabbage ExplodingCabbage self-assigned this Feb 28, 2025
…eful for compatibility with something, somewhere
Doesn't seem to really affect anything. Output in dist/diff.js is almost identical; just formatting changes and a handful of other things that look inconsequential.
…n into the function comments, so it can show up in people's editors
@ExplodingCabbage ExplodingCabbage marked this pull request as ready for review April 11, 2025 13:47
@ExplodingCabbage
Copy link
Collaborator Author

ExplodingCabbage commented Apr 11, 2025

I think this is ready to merge. But goddamn, it was hard. Everything about this process was like waaaaay harder than expected, but most especially

  • actually crafting roughly-correct types given both the nuance of how different possible options affect function return types in the diffFoo & createPatch functions and also the obstacle that TypeScript's buggy matching of calls to overload signatures meant that some equally-correct possibilities were mysteriously broken
  • figuring out how to 1. avoid breaking any previously-supported ways of importing stuff while also 2. getting the types working for all module systems and arethetypeswrong giving my work its blessing. In particular:
    • I expected the right way to do things would be able to generate an ESM build, a CJS build, and a shared type declarations build that would be used as the types for both the ESM and CJS builds... but this seems almost impossible to do in a way that isn't broken (or at least that doesn't make attw howl with outrage - I wasn't always convinced its complaints were genuine problems, but also wasn't confident enough to ignore them), so instead I have identical copies of the types inside the libesm and libcjs folders. Seems weird, but whatever.
    • it seems to be outright impossible to use a folder mapping without an asterisk in exports in a way that attw won't error out about, but we need it to support old versions of Node, so I just had to use --entrypoints to tell attw to ignore it.

Probably there were multiple other things I banged my head against a wall for hours on, too - it's been a long time since I started work on this and I've forgotten things.

I've tried to review my own code and I reckon I'm happy merging and shipping this, but I'm going to wait at least a week. I'd be very grateful if anyone with more TypeScript expertise than me would be willing to review my work. @isaachinman & @andrewbranch, you seem like obvious candidates though of course you are not under any obligation at all to spend time looking at this.

@isaachinman
Copy link

Not sure I will have time to review a +3,647 −1,480 PR, but if you can release a canary version, I can install and check for type correctness/type errors.

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.

Type for Intl.Segmenter Please ship TypeScript definitions
2 participants