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

Don't make Tagged type compatibility dependent on type fest version #875

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

Conversation

ethanresnick
Copy link
Contributor

@ethanresnick ethanresnick commented Apr 29, 2024

Since removing Opaque/UnwrapOpaque is obviously gonna require a new major version, I wanted to put this PR up now, in case it makes sense to land both changes in the same major version.

This PR switches the tag key used by the Tagged type to be a string rather than a symbol, undoing this commit. The rationale for that, which I've summarized from this comment is:

  1. It's easy for two different copies of type-fest to end up installed in a project. Consider a dependency tree like:

    @my-company/app
    ├─┬ @my-company/types
    │ └── type-fest
    ├─┬ @my-company/some-internal-library
    │ └─┬ @my-company/types
    │   └── type-fest
    └── type-fest
    

    Depending on the version of type-fest installed at the root (i.e., by the @my-company/app package) and the version of the @my-company/types package used the chosen version of @my-company/some-internal-library, npm may be forced to install multiple type-fest copies.

  2. When multiple type-fest copies are present, a Tagged type produced by one copy is not assignable to a Tagged type produced by another copy even if the types have the same tag names, tag metadata, and base/untagged type, because Typescript sees the tag symbol from each copy of type-fest as distinct.

    This is weird behavior, as it ends up making assignability dependent on exactly how/when npm chooses to dedupe packages, which isn't even necessarily stable as new packages are installed or as npm is updated or as installed packages are updated.

    In the example dependency tree above, it means that: if @my-company/some-internal-library returns a tagged type from its copy of the @my-company/types package, that type may not be assignable to the copy of that same type exported by the copy of @my-company/types that @my-company/app is depending on directly — even if the tagged type hasn't changed (in terms of its tag names/tag metadata/base type).

This also happens to make Record<string, T> taggable (solving #708), at the expense of Record<symbol, T> not being taggable (which seems like a good tradeoff).

Given that type-fest used a string-based key for many years, I don't think this poses any big issues, but there would be some small user-facing changes. For example, keyof Tagged<{ foo: string }, 'SomeTag'> would become 'foo' | '__type-fest-tag__' instead of 'foo' | typeof tag; that's not fundamentally different, but it's maybe a tad less clear. It would probably be helpful too, at that point, for type-fest to export a string literal type with the name of the magic tag string (i.e., "__type-fest-tag__"), so that users can do Exclude<keyof Tagged, TypeFestTagKey>, in much the same way they might've had to do Exclude<keyof Tagged, symbol> before.

@ethanresnick ethanresnick force-pushed the 04-29-feat_don_t_make_tagged_type_compatibility_dependent_on_type-fest_version branch from 613761a to ce16ccb Compare April 29, 2024 08:43
@ethanresnick
Copy link
Contributor Author

Perhaps a better, less-disruptive approach to the same ends: publish a new package that just has the tag symbol, and commit to never updating the version of that package; that way, it will only be installed once, and every copy of type-fest will reference the same symbol.

@sindresorhus
Copy link
Owner

Perhaps a better, less-disruptive approach to the same ends: publish a new package that just has the tag symbol, and commit to never updating the version of that package; that way, it will only be installed once, and every copy of type-fest will reference the same symbol.

Good idea. Done here: https://github.com/sindresorhus/tagged-tag

The package is a ESM module, so it will not work until type-fest targets ESM, which it will for v5. I have published a 0.1.0 in the meantime to provide feedback. If it's good, I will do a 1.0.0 before merging this PR, and at that point, that package is locked forever.

@belgattitude
Copy link

belgattitude commented May 4, 2024

I’m wondering how safe it is with a locked external package ?

In the past I’ve seen bugs in package managers in the way they hoist a same exact version at different paths. Happened mostly in monorepos with yarn or pnpm. In general a dedupe would fix it but sometimes it goes further (older versions of pnpm or some specific settings). In general it’s safe when using recent pm, but it still comes back from time to time.

Just an open remark. That said I like the approach

@ethanresnick
Copy link
Contributor Author

I think you're right @belgattitude that using an external package may not solve this in some edge cases, or may occasionally require the user to do a manual npm dedupe (or other package manager equivalent).

At the very least, we have to lock both the minor + patch versions of the external package, not just its major version, because of npm's behavior described here:

During the installation process, the [email protected] dependency for b was placed in the root of the tree. Though d's dependency on [email protected] could have been satisfied by [email protected], the newer [email protected] dependency was used, because npm favors updates by default, even when doing so causes duplication.

All that said, I think an external package will solve this in the vast majority of cases, and we know that applying this approach in type-fest v5 is totally non-breaking (as a user installing type-fest v5 would've gotten a new copy with a new symbol anyway). So I think we should do this non-breaking, minimally-invasive approach first, and then only switch to a string key if it proves inadequate.

@ethanresnick ethanresnick force-pushed the 04-29-feat_don_t_make_tagged_type_compatibility_dependent_on_type-fest_version branch from ce16ccb to 21fbe7b Compare May 6, 2024 18:18
@ethanresnick ethanresnick force-pushed the 04-29-feat_don_t_make_tagged_type_compatibility_dependent_on_type-fest_version branch from 21fbe7b to c975c2b Compare May 6, 2024 18:21
@ethanresnick
Copy link
Contributor Author

ethanresnick commented May 6, 2024

@sindresorhus I've updated this to use the new package. I'm not sure how to test it though?

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.

3 participants