joe-bell / cva

Class Variance Authority
https://cva.style
Apache License 2.0
5.66k stars 109 forks source link

TS2742 - inferred type cannot be named without a reference #163

Closed erictaylor closed 1 year ago

erictaylor commented 1 year ago

Describe the bug The following TS error is encountered when attempting to build a library with class-variance-authority@0.6.0:

The inferred type of 'someCVAVariable' cannot be named without a reference to '../../../node_modules/class-variance-authority/dist/types'. This is likely not portable. A type annotation is necessary.ts(2742)

Similar to issue: https://github.com/joe-bell/cva/issues/27

TypeScript version: 5.0.4

To Reproduce Steps to reproduce the behavior:

  1. Use TSConfig with declaration: true setting.
  2. Create an export utility that uses VariantProps and calls a declared cva() function. See screenshot.

Expected behavior No TS Error.

Screenshots

Screenshot 2023-05-23 at 9 36 39 PM Screenshot 2023-05-23 at 9 39 28 PM

TypeScript Version 5.0.4

Additional context The problem is that ClassProp, ClassValue, OmitUndefined, StringToBoolean types are not ultimately exported from the class-variance-authority library. If you look at the index.d.ts file you can see those types are imported and referenced in other types, but they ultimately aren't exported themselves (see the last line in of index.d.ts of export {}.

Simply exporting those types fixes the issue.

ie

diff --git a/dist/index.d.ts b/dist/index.d.ts
index 676e466a43ad8932cbb3131bb2c3dea687d47041..26e2c538a42fff5ab15a6e584c48ce6001fda86a 100644
--- a/dist/index.d.ts
+++ b/dist/index.d.ts
@@ -18,4 +18,4 @@ type Config<T> = T extends ConfigSchema ? {
 } : never;
 type Props<T> = T extends ConfigSchema ? ConfigVariants<T> & ClassProp : ClassProp;
 export declare const cva: <T>(base?: ClassValue, config?: Config<T> | undefined) => (props?: Props<T> | undefined) => string;
-export {};
+export { ClassProp, ClassValue, OmitUndefined, StringToBoolean };

Happy to submit a PR.

joe-bell commented 1 year ago

Damn! It's super frustrating that this issue is still cropping up 😢

Seems very much like an upstream issue with TypeScript (https://github.com/microsoft/TypeScript/issues/47663)

I really don't want to export these types as they aren't for public use. There must be another way to solve this? The only alternative I can think of is dropping .ts altogether and going full JSDoc at this point

If there isn't an alternative, then I'd probably suggest we close this and contribute to that TypeScript thread (https://github.com/microsoft/TypeScript/issues/47663)

erictaylor commented 1 year ago

@joe-bell You can additionally also fix this issue by updating the package.json's exports as follows:

{
  "exports": {
    ".": {
      "import": "./dist/index.mjs",
      "require": "./dist/index.js",
      "types": "./dist/index.d.ts"
    },
    "./types": {
      "types": "./dist/types.d.ts"
    }
  },
}
joe-bell commented 1 year ago

Thanks for the suggestion @erictaylor

Unfortunately, this still means I'm exposing private types publicly

erictaylor commented 1 year ago

Personally I don't see the big deal with exposing these types, but to each our own.

Other tested alternatives that work that would keep those types "private" would be:

  1. Move the types into the index.ts file and not have them in a types.ts file that isn't linked via package exports.
  2. Move all the types to the types.ts file, that way private types never have to be exported (only exporting the public types), and then appropriately expose the types file in package.json exports OR re-export the public types in index.
  3. Remove exports from package.json so the old fallback "types", "main", "module" is used.
  4. Use a library like dts-bundle to build the types all into one index.d.ts file.
erictaylor commented 1 year ago

I've already found myself wanting to be able to use the un-exposed "Props" type and not being able to access it.

VariantProps isn't sufficient for some cases.

const exampleCVA = cva('foo', { variants: { intent: { bar: 'xaz' } } });

type ExampleVariants = VariantProps<typeof exampleCVA>;

const example = (variants: ExampleVariants): string => {
  return someOtherWrapperUtil(exampleCVA(variants));
};

// Will complain that `className` isn't a prop, even though exampleCVA() can take it.
// Obviously the typing for VariantProps only returns the variants... but in this case I want everything supported.
example({ intent: 'bar', className: 'some-other-class' });
joe-bell commented 1 year ago

className and class types aren't exported because they're designed to be framework agnostic.

Instead, I would strongly recommend using React's types (or your preferred framework)


However, this is off-topic to the issue at hand. If there doesn't seem to be a way to circumvent this without exporting types, I suggest we contribute to https://github.com/microsoft/TypeScript/issues/47663 to ensure the wider TypeScript community benefits

erictaylor commented 1 year ago

className and class types aren't exported because they're designed to be framework agnostic.

Instead, I would strongly recommend using React's types (or your preferred framework)

I understand that. But the point I was arriving to is similar to the point that https://github.com/joe-bell/cva/issues/27#issuecomment-1089558744 arrived too

A benefit of exporting the types is that it makes it easier to extend on cva. For example, I spend most of my time with React, so when using cva I will have to map the className prop to the class prop. It would be nice if I could use those types to make my own cva wrapper that takes className instead. Whilst I'm sure it is possible currently, it would be easier if the types that make up cva were made available.

In other words, your linked example, just straight up consumes the cva return function, which is typed to take a className and there is no problem. However, if you write your own wrapper (like suggested in the docs for utilizing twMerge), now you can no longer pass className or class if you just used VariantProps<T>, you'd have to addtionally add { className?: string; } (which is easy enough but it would be more convenient from a consumer standpoint if the library exported the used types so extending in this way was more straight forward and could match the original API more closely (supporting either class or className but not both props).


However, this is off-topic to the issue at hand. If there doesn't seem to be a way to circumvent this without exporting types, I suggest we contribute to https://github.com/microsoft/TypeScript/issues/47663 to ensure the wider TypeScript community benefits

I deeply appreciate you open sourcing this library, your contribution to the community, and your response times on this issue.

Look, I get it, this is your library and you get to make the call on these things. But as a consumer, this issue is a non-starter for being able to use this library. The linked TypeScript issue is over a year old, and I don't see the linked problem going away from that side anytime soon.

I'm happy to provide PR to this repo with any of the various options that are available for fixing this issue.

If the final resolution you want to give this issue is to wait on https://github.com/microsoft/TypeScript/issues/47663, feel free to close this issue out and I'll happily build my own solution.

joe-bell commented 1 year ago

I understand you’re frustrated by this issue and your other concerns, but it’s only been 13 hours!

All I’m asking for here is for some due diligence, rather than a hotfix that will expose private types and ultimately break semver

I would appreciate your support in investigating a solution to this problem that doesn’t compromise the stability of this project

joe-bell commented 1 year ago

Back on track, one alternative to explore: we could move these types into a file which clearly recommends against use until the upstream issue is resolved?

Something like cva/internal-please-do-not-use

That way they’re exported but perhaps this level of documentation is clear enough to steer against misuse?

As for the className prop, we could explore something like an AllProps or Props utility?

erictaylor commented 1 year ago

I understand you’re frustrated by this issue and your other concerns, but it’s only been 13 hours!

I apologize, that isn't my intent at all.


...one alternative to explore: we could move these types into a file which clearly recommends against use until the upstream issue is resolved?

Something like cva/internal-please-do-not-use

That way they’re exported but perhaps this level of documentation is clear enough to steer against misuse?

That is definitely an option. You could take it a step further if needed and add JSDoc comments on those types as well to explicitly outline the reason for them being currently exported but considered private.

Would just need to ensure that file was added to the exports map.

As for the className prop, we could explore something like an AllProps or Props utility?

100%.

Something that simply:

export type ComponentFunctionProps<T extends (...args: any) => any> = VariantProps<T> & ClassProp; 

Again, you could also just move the types from the types.ts file into index.ts and nuke the types.ts file all together. You lose the organization, but then you can keep those private types "private". Adding JSDoc comments to the types about the desire to keep them "private" could be added to help contributors know they are intended to never be exported.

Just an alternative suggestion.

joe-bell commented 1 year ago

Again, you could also just move the types from the types.ts file into index.ts and nuke the types.ts file all together.

Frustratingly, the reason why this was done was to avoid this exact problem! (#29)

joe-bell commented 1 year ago

I've tried reverting the decision to put all types back into index.ts, but sadly the issue still persists

joe-bell commented 1 year ago

I have dug into this further as it's blocking my exploration of #160 – none of the options above proposed work for me I'm afraid

Unsure how to continue at this point

erictaylor commented 1 year ago

I wonder what would happen if the following was removed (from package.json):

{
  "peerDependencies": {
    "typescript": ">= 4.5.5 < 6"
  },
  "peerDependenciesMeta": {
    "typescript": {
      "optional": true
    }
  }
}

I'm using PNPM, and I notice a lot of the comments in the linked TypeScript thread seem also to be PNPM related. I noticed that the way this package is linked is a bit different than I would expect, and I'm wondering in this optional peer dependency (which is automatically installed via PNPM) is messing things up.

Not a guarantee, but the only significant thing that stands out to me about this package.

joe-bell commented 1 year ago

I was sceptical of this suggestion but went ahead and tried anyway: sadly still no luck 😢

joe-bell commented 1 year ago

Some good news, I've managed to resolve this issue by 2 steps that seem to be interlinked:

  1. Moving into a single index.ts
  2. Duplicating clsx's types rather than importing them

Before I commit to this strategy, I want to see if using @JSDoc suffers from the same issue

joe-bell commented 1 year ago

After exploring this for some time now, I think the best strategy forwards is to land the fix in cva@1.0 – integrating this earlier will make things more complicated right now

I don't have an ETA, but cva is my only open-source focus right now

For the short-term, I recommend patching around this 🙏

Thanks again for raising this!