JedWatson / react-select

The Select Component for React.js
https://react-select.com/
MIT License
27.49k stars 4.12k forks source link

fix: Add declarations export to package.json #5762

Closed csandman closed 10 months ago

csandman commented 10 months ago

This is an attempt to fix #5743, in which the module augmentation described in the docs no longer works in projects using "moduleResolution": "bundler" | "node16" | "nodenext". As far as I can tell, it's because projects using those settings look for the exports field in the package.json, which is more strict about which file paths can be accessed in the dependency package. I'm honestly not the most familiar with the interactions between TypeScript, ES Modules, and module augmentation (the entire node package ecosystem is hard to keep track of haha), but this fix did the trick for me when I tested it in my own project chakra-react-select by modifying the react-select package.json manually.

I know that @FabianFrank already had a similar PR up for this fix (#5744) but it wouldn't actually build because preconstruct saw the exports config as invalid. I was able to fix this by adding the following to the preconstruct config:

    "exports": {
      "extra": {
        "./dist/declarations/src/*": {
          "types": "./dist/declarations/src/*.d.ts"
        }
      }
    }

I extended the export statement to use a wildcard as well, because in my project I'm also extending the types for the MultiValue props, as well as the indicators props, so hopefully we can stick with that. I'm not exactly sure how the path structure is supposed to work, but doing it like this also allowed for augmenting the nested declarations like "react-select/dist/declarations/src/components/MultiValue".

Anyway, I'm really not sure if this is the best approach to fixing this issue, but users of my package have been having issues with this for a while now before I finally discovered that this must be the issue, so it would be great if a fix for this could get merged in.


On a side note, this could be an opportunity to simplify the import path people have to use with something like this:

    "./declarations/*": {
      "types": "./dist/declarations/src/*.d.ts"
    }

Which would allow people to simplify their module augmentation to this:

declare module "react-select/declarations/Select" {
  export interface Props<Option, IsMulti extends boolean, Group extends GroupBase<Option>> {
    // ...
  }
}

However, this would only work for those whose projects are actually using the exports field, which would create a divergence in the guide for how to accomplish adding custom props with module augmentation, so I figured it was probably best to keep them consistent for now.


EDIT:

I actually went ahead and made two example CodeSandboxes for the original issue, one highlighting how the module augmentation is broken with the current version of this package v5.7.5, and another with the version generated by this PR showing it fixed:

Both of these will appear fixed in the generated CodeSandboxes below.

changeset-bot[bot] commented 10 months ago

🦋 Changeset detected

Latest commit: 5908f9970891cc8abc0f053a94652bc07c1eca9c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package | Name | Type | | ------------ | ----- | | react-select | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

codesandbox-ci[bot] commented 10 months ago

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5908f9970891cc8abc0f053a94652bc07c1eca9c:

Sandbox Source
react-codesandboxer-example Configuration
react-select broken module augmentation with moduleResolution="bundler" PR
react-select fixed module augmentation with moduleResolution="bundler" PR
FabianFrank commented 10 months ago

Nice! 👍 I’ll close my PR since this is definitely better with a working build.

csandman commented 10 months ago

Nice! 👍 I’ll close my PR since this is definitely better with a working build.

Sounds good, hopefully this gets some attention! I tried using patch-package on React Select's package.json in my own project to fix this, but as far as I know that doesn't really work right on a package.json. Plus, other people could probably benefit from this change.

FabianFrank commented 10 months ago

I tried using patch-package on React Select's package.json in my own project to fix this, but as far as I know that doesn't really work right on a package.json.

It does work if you pass --exclude when creating the patch, e.g. patch-package react-select --exclude.

csandman commented 10 months ago

It does work if you pass --exclude when creating the patch, e.g. patch-package react-select --exclude.

Yeah I was able to actually create the patch, but it didn't appear to be sticking around after I installed my package in an app. That approach would probably work for someone who has react-select as a direct dependency though, like a normal web app.

You can see what I'm talking about with this PR I attempted:

I don't understand why it's not working exactly, but this comment sort of explains what the package.json isn't included by default.

csandman commented 10 months ago

@JedWatson @lukebennett88 @Methuselah96 Could someone take a look at this? It's a really small change that I can't image any possible regressions existing for. And it's a big pain point at the moment for anyone using the newer moduleResolution TS config that also needs to augment the select types.

emmatown commented 10 months ago

Thanks for the PR but these internal files aren't really intended to be public API and be augmented directly like this so I don't think this makes sense to merge.

The various types here can already be augmented by using the place where they're exported publicly. e.g. the Props type can already be augmented via react-select/base

import Select, { GroupBase } from "react-select";
import type {} from "react-select/base";

declare module "react-select/base" {
  export interface Props<
    Option,
    IsMulti extends boolean,
    Group extends GroupBase<Option>
  > {
    something?: string;
  }
}

<Select something="" />;

https://codesandbox.io/s/react-select-module-augmentation-with-moduleresolution-bundler-fixed-fxpczr?file=/src/App.tsx

floriancargoet commented 10 months ago

Thanks! It seems the key to making it work with "react-select/base" is the empty import import type {} from "react-select/base";. Without it, TS doesn't find the module for augmentation.

@emmatown If augmenting the internal file is not supported, the official docs should be updated because that's exactly what is recommended. https://react-select.com/typescript#custom-select-props

lukebennett88 commented 10 months ago

Good point @floriancargoet. I've just opened a PR to update the docs: https://github.com/JedWatson/react-select/pull/5776

csandman commented 10 months ago

Thanks for the PR but these internal files aren't really intended to be public API and be augmented directly like this so I don't think this makes sense to merge.

The various types here can already be augmented by using the place where they're exported publicly. e.g. the Props type can already be augmented via react-select/base

@emmatown Thanks for this info. I had actually tried a few different ways of making this work without the nested augmentation but didn't realize you could do it from react-select/base. I had tried doing it from the root react-select, but that didn't end up augmenting the internal props, only the exported type. And like @floriancargoet mentioned, the nested augmentation has been the recommended approach for a while, so getting that updated in the docs would be great. Thanks for the alternative PR @lukebennett88!

One other question though, can anyone explain why the empty import import type {} from "react-select/base"; is required for the types to be found properly? I saw this pattern a bit in my research (well I saw an empty export {} from ... more), and I'm curious why this is a thing. Is it new with these newer moduleResolution types?

csandman commented 10 months ago

Also, I had one last question, with this new approach to the module augmentation, is it possible to augment any of the other prop types? For example, in my project, I'm extending the LoadingIndicatorProps interface to add some extra custom props to it. I'm doing this because my package replaces all of the internal react-select components, using Chakra UI components in their place. And I'm allowing my users to wrap them similarly to the core package allows for with the <components.LoadingIndicator> approach (except mine is <chakraComponents.LoadingIndicator>).

Here is an example of what I'm doing in my current version: https://github.com/csandman/chakra-react-select/blob/3d9c4fc95ecb9e72a2943e6d94f11612b9aa637d/src/module-augmentation.ts#L227-L287

I tried augmenting all of the more specific component types from react-select/base, but it appears that none of the specific component types are being used from here, so that's not possible. Is there an alternative approach that I'm missing, or is the idea that these types are just not meant to be modified?

emmatown commented 10 months ago

One other question though, can anyone explain why the empty import import type {} from "react-select/base"; is required for the types to be found properly?

It's just to include the module in the TypeScript project so TypeScript is aware of it and it can be augmented. Any other sort of import of react-select/base would also work fine

Also, I had one last question, with this new approach to the module augmentation, is it possible to augment any of the other prop types? For example, in my project, I'm extending the LoadingIndicatorProps interface to add some extra custom props to it.

LoadingIndicatorProps and others are exported from the root at react-select so you can do this:

declare module "react-select" {
  export interface LoadingIndicatorProps<
    Option = unknown,
    IsMulti extends boolean = boolean,
    Group extends GroupBase<Option> = GroupBase<Option>
  > {
    something: string;
  }
}
csandman commented 10 months ago

It's just to include the module in the TypeScript project so TypeScript is aware of it and it can be augmented. Any other sort of import of react-select/base would also work fine

Gotcha, thanks for the reply! That makes sense.

LoadingIndicatorProps and others are exported from the root at react-select so you can do this:

Wow, can't believe I missed this. I could have sworn I had tried this in the past and it didn't work, but maybe I didn't. Either way, thanks a lot for the solution! Seems like all of my problems are solved!

floriancargoet commented 10 months ago

Another tip for anyone landing on this issue: If you're building a library, the empty import might be removed by your bundler, and the apps consuming your lib & .d.ts wouldn't see your augmentation. That's what happened to me. I've solved it by exporting the type, ensuring it wouldn't be removed.

export type { Props as ReactSelectProps } from "react-select/base";