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

fix: Relax constraints on @react-spring/web #2280

Merged
merged 3 commits into from
Apr 25, 2023

Conversation

dubzzz
Copy link
Contributor

@dubzzz dubzzz commented Mar 29, 2023

Having a constraint on @react-spring/web blocking it to a very precise version (here: 9.4.5) can push users of nivo into pulling it several times in their node_modules or bundles if they don't really check for duplicated packages. Indeed it's rather simple to have two distinct libraries requesting it at version 9.x at the same time, and if one of them (not nivo) request something not being 9.4.5 they we would pull the library twice.

It seems that given @react-spring/web seems to follow semver, there is not real risk to say nivo supports ^9.4.5 instead of 9.4.5 only.

Having a constraint on `@react-spring/web` blocking it to a very precise version (here: `9.4.5`) can push users of nivo into pulling it several times in their node_modules or bundles if they don't really check for duplicated packages. Indeed it's rather simple to have two distinct libraries requesting it at version `9.x` at the same time, and if one of them (not nivo) request something not being `9.4.5` they we would pull the library twice.

It seems that given `@react-spring/web` seems to follow semver, there is not real risk to say nivo supports `^9.4.5` instead of `9.4.5` only.
@dubzzz
Copy link
Contributor Author

dubzzz commented Mar 29, 2023

Seems that I have some pieces of the code to adapt if I want this Pr to make it and at least compile:

packages/treemap/src/defaults.ts:69:14 - error TS4023: Exported variable 'htmlDefaultProps' has or is using name 'AnimationConfig' from external module "/tmp/9a4cf066/node_modules/@react-spring/core/dist/index" but cannot be named.

69 export const htmlDefaultProps = {

dubzzz added a commit to dubzzz/react-spring that referenced this pull request Mar 29, 2023
Some open-source and largely used libraries such as nivo directly bind to the types being declared by `@react-pring/core` in some of their code.

For instance in nivo, the following code:

```ts
import { SpringValues } from '@react-spring/web'

//...
export interface NodeProps<Datum extends object> {
    //...
    animatedProps: SpringValues<NodeAnimatedProps>
    //...
}
```

Implicitely force nivo to be able to reference parts of the typings of `@react-spring/core` into their d.ts files if they want to be able to publish their package. It basiaclly results into `Partial<import("@react-spring/core").AnimationConfig>` in the produced d.ts at some point. And as a consequence TypeScript complains with the error `error TS4023: Exported variable 'htmlDefaultProps' has or is using name 'AnimationConfig' from external module "/tmp/9a4cf066/node_modules/@react-spring/core/dist/index" but cannot be named.`.

The proposed change exposes `AnimationConfig` as a type, meaning we still don't expose the class, just its type. It would help into making nivo able to support recent version of `@react-spring/*`. See related issue on nivo's side: plouc/nivo#2280
@dubzzz
Copy link
Contributor Author

dubzzz commented Mar 29, 2023

This PR is waiting for a status on pmndrs/react-spring#2128. If the other PR gets merged then nivo could probably start declaring this import with a ^. In the meantime, maybe I should change from 9.4.5 to the version that will have the patch in it for TS users 🤔

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 7, 2023

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 b40fb6f:

Sandbox Source
nivo Configuration

@dubzzz
Copy link
Contributor Author

dubzzz commented Apr 7, 2023

@plouc Any opinion?

@plouc
Copy link
Owner

plouc commented Apr 25, 2023

@plouc Any opinion?

9.4.5 || ^9.7.2 sounds good.

Copy link
Owner

@plouc plouc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@plouc plouc merged commit 6d94660 into plouc:master Apr 25, 2023
@dubzzz dubzzz deleted the propose-bump-spring branch April 25, 2023 16:30
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.

2 participants