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

feat: Move @testing-library/dom and @types/react-dom to peer dependencies #1305

Merged
merged 4 commits into from
Jun 3, 2024

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Apr 22, 2024

Closes #1184
Docs: testing-library/testing-library-docs#1384

BREAKING CHANGE:
@testing-library/dom was moved to a peer dependency and needs to be explicitly installed. This reduces the chance of having conflicting versions of @testing-library/dom installed that frequently caused bugs when used with @testing-library/user-event. We will also be able to allow new versions of @testing-library/dom being used without a SemVer major release of @testing-library/react by just widening the peer dependency.
@types/react-dom needs to be installed if you're typechecking files using @testing-library/react.

Type dependencies need to match their runtime counterpart.
If foo is a dependency, @types/foo needs to be one as well.
If foo is a peer dependency, @types/foo needs to be one as well.
This is especially apparent if the constraint on foo spans multiple major versions.
If we'd make @types/foo a direct dependency, users couldn't control which major version they get.
Package managers would pick the highest.
By moving @types/foo to peer dependencies, users can control which version of @types/foo they have.

In our case specifically, you'd get type mismatches of @types/react-dom if we just widen the peer dependency constraint from ^18 to ^18 | ^19. Currently, we're already compatible with the planned React 19 features so all we'd have to do is widen the constraint on release.

Copy link

codesandbox-ci bot commented Apr 22, 2024

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

Sandbox Source
react-testing-library-examples Configuration

MatanBobi
MatanBobi previously approved these changes Apr 23, 2024
Copy link
Member

@MatanBobi MatanBobi left a comment

Choose a reason for hiding this comment

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

Thanks.
Just a thought since we're doing this and bumping a major - lately we're having many issues with DTL version mismatches in RTL and user-event, do we maybe want to think about also move DTL to peerDeps?

@eps1lon
Copy link
Member Author

eps1lon commented Apr 23, 2024

do we maybe want to think about also move DTL to peerDeps?

I think so. Since DTL is stateful due to the config object, having multiple versions is problematic.

Ideally we wouldn't have the stateful config object to begin with but that requires a large refactor that needs coordination.

We don't need to rush this in this PR though. Could you help out and prepare docs and a commit making that change?

@MatanBobi
Copy link
Member

We don't need to rush this in this PR though. Could you help out and prepare docs and a commit making that change?

Sure thing, I'll get to it later this week.

@MatanBobi
Copy link
Member

MatanBobi commented Apr 24, 2024

@eps1lon Wdyt about making this change as part of a major too? #1277

Also, here's the PR to move DTL to peer: #1309

@eps1lon
Copy link
Member Author

eps1lon commented Apr 24, 2024

Also, here's the PR to move DTL to peer: #1309

Just push this commit to this branch. I don't want to go through the alpha branch shenanigans and just ship the major with one PR.

@MatanBobi
Copy link
Member

Just push this commit to this branch. I don't want to go through the alpha branch shenanigans and just ship the major with one PR.

Done. I haven't fixed the conflicts though as we have more changes coming to that file.

eps1lon added 2 commits April 26, 2024 13:07
Type dependencies need to match their runtime counterpart.
If `foo` is a dependency, `@types/foo` needs to be one as well.
If `foo` is a peer dependency, `@types/foo` needs to be one as well.
This is especially apparent if the constraint on `foo` spans multiple major versions.
If we'd make `@types/foo` a direct dependency, users couldn't control which major version they get.
Package managers would pick the highest.
By moving `@types/foo` to peer dependencies, users can control which version of `foo` and  `@types/foo` they have.
commit 2fa5eac
Author: Matan Borenkraout <[email protected]>
Date:   Wed Apr 24 11:06:27 2024 +0300

    fix formatting

commit 7a07982
Author: Matan Borenkraout <[email protected]>
Date:   Wed Apr 24 09:14:25 2024 +0300

    feat!: move dtl to peerDeps
@MatanBobi
Copy link
Member

@eps1lon Do we want to push this one forwards after seeing that the latest major release looks ok?

@eps1lon
Copy link
Member Author

eps1lon commented May 28, 2024

We haven't got any reports recently so this seems safe now. I don't expect bugs that require big changes i.e. if something comes up it's likely small enough to warrant a backport.

Thank you for pushing this @MatanBobi

@MatanBobi MatanBobi changed the title feat: Move @types/react-dom to optional peer dependencies feat!: Move @types/react-dom to optional peer dependencies Jun 1, 2024
@MatanBobi
Copy link
Member

Thanks @eps1lon. I've merged from main and fixed the conflicts we had so this is ready to be merged IMO.

@eps1lon eps1lon changed the title feat!: Move @types/react-dom to optional peer dependencies feat!: Move @testing-library/dom and @types/react-dom to peer dependencies Jun 3, 2024
@eps1lon
Copy link
Member Author

eps1lon commented Jun 3, 2024

Does feat! do anything for semantic-release?

@MatanBobi
Copy link
Member

Does feat! do anything for semantic-release?

AFAIU no, it only uses the BREAKING CHANGE commit message, though I'm trying to follow conventional commit messages:
https://www.conventionalcommits.org/en/v1.0.0/#commit-message-with--to-draw-attention-to-breaking-change

You can feel free to remove it if you want.

@eps1lon eps1lon changed the title feat!: Move @testing-library/dom and @types/react-dom to peer dependencies feat: Move @testing-library/dom and @types/react-dom to peer dependencies Jun 3, 2024
@eps1lon
Copy link
Member Author

eps1lon commented Jun 3, 2024

We're using semantic-release so we have to stick with what that tool understands.

@eps1lon eps1lon merged commit a4744fa into testing-library:main Jun 3, 2024
10 checks passed
@eps1lon eps1lon deleted the peer-types branch June 3, 2024 13:52
@ph-fritsche
Copy link
Member

Does feat! do anything for semantic-release?

If you like to switch to conventional commit, it's the default config here:

      - name: 🚀 Release
        uses: ph-fritsche/action-release@v2
        env:
          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
          NPM_TOKEN: ${{ secrets.NPM_TOKEN }}

@eps1lon
Copy link
Member Author

eps1lon commented Jun 3, 2024

Existing setup works for us so I'd rather not experiment.

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 conflicts in testing-library vs react
3 participants