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.

Sign up for GitHub

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

Jump to bottom

[v5] breaking: make React 18 as minimal requirement #2236

Merged
merged 5 commits into from
Dec 17, 2023
Merged

[v5] breaking: make React 18 as minimal requirement #2236

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
781df93
fix: update package.json to require react 18+
charkour Dec 9, 2023
4f17d4e
chore: update github actions to test on react 18+
charkour Dec 9, 2023
0a83fab
Merge remote-tracking branch 'pmndrs/v5' into v5-require-react-18
charkour Dec 14, 2023
394b7a6
chore: remove devtools-skip hack from actions
charkour Dec 17, 2023
b4742b7
chore(test): remove CI-SKIP from devtools tests
charkour Dec 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 3 additions & 7 deletions .github/workflows/test-multiple-builds.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ jobs:
strategy:
fail-fast: false
matrix:
build: [cjs, umd] # [cjs, esm, umd, system]
env: [development, production]
build: [cjs, esm, umd] # [cjs, esm, umd, system]
charkour marked this conversation as resolved.
Show resolved Hide resolved
env: [development] # [development, production]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dai-shi, I copied this change from https://github.com/pmndrs/valtio/pull/819/files, but looking at this, could it be a mistake? I'd like to test both development and production in the testing matrix. Should this be changed to the following?

Suggested change
env: [development] # [development, production]
env: [development, production]

devtools-skip:
- CI-MATRIX-NOSKIP
include:
Expand All @@ -39,10 +39,6 @@ jobs:
- run: git fetch --depth=1 origin +refs/tags/*:refs/tags/*
- run: yarn install --frozen-lockfile --check-files
- run: yarn build
- name: Use React 17 for production test
if: ${{ matrix.env == 'production' }}
run: |
yarn add -D [email protected] [email protected] @testing-library/[email protected]
- name: Patch for DEV-ONLY
if: ${{ matrix.env == 'development' }}
run: |
Expand All @@ -60,7 +56,7 @@ jobs:
- name: Patch for ESM
if: ${{ matrix.build == 'esm' }}
run: |
sed -i~ "s/<rootDir>\/src\(.*\)\.ts/<rootDir>\/dist\/esm\1.js" package.json
sed -i~ "s/<rootDir>\/src\(.*\)\.ts/<rootDir>\/dist\/esm\1.js/" package.json
charkour marked this conversation as resolved.
Show resolved Hide resolved
sed -i~ "1s/^/import.meta.env=import.meta.env||{};import.meta.env.MODE='${NODE_ENV}';/" tests/*.tsx
env:
NODE_ENV: ${{ matrix.env }}
Expand Down
26 changes: 6 additions & 20 deletions .github/workflows/test-multiple-versions.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@ jobs:
fail-fast: false
matrix:
react:
- 16.8.0
- 17.0.0
- 18.0.0
- 18.1.0
- 18.2.0
Expand All @@ -39,18 +37,18 @@ jobs:
- CI-MATRIX-NOSKIP
include:
- devtools-skip: CI-MATRIX-[2345]
react: 16.8.0
react: 18.0.0
charkour marked this conversation as resolved.
Show resolved Hide resolved
- devtools-skip: CI-MATRIX-[1345]
react: 16.8.0
react: 18.0.0
- devtools-skip: CI-MATRIX-[1245]
react: 16.8.0
react: 18.0.0
- devtools-skip: CI-MATRIX-[1235]
react: 16.8.0
react: 18.0.0
- devtools-skip: CI-MATRIX-[1234]
react: 16.8.0
react: 18.0.0
exclude:
- devtools-skip: CI-MATRIX-NOSKIP
react: 16.8.0
react: 18.0.0
steps:
- uses: actions/checkout@v2
- uses: actions/setup-node@v2
Expand All @@ -59,18 +57,6 @@ jobs:
cache: yarn
- run: git fetch --depth=1 origin +refs/tags/*:refs/tags/*
- run: yarn install --frozen-lockfile --check-files
- name: Install legacy testing-library
if: ${{ startsWith(matrix.react, '16.') || startsWith(matrix.react, '17.') }}
run: yarn add -D @testing-library/[email protected]
- name: Patch for React 16
if: ${{ startsWith(matrix.react, '16.') }}
run: |
sed -i~ '1s/^/import React from "react";/' tests/*.tsx
sed -i~ 's/"jsx": "react-jsx"/"jsx": "react"/' tsconfig.json
sed -i~ 's/import\.meta\.env[?]\.MODE/"DEVELOPMENT".toLowerCase()/' src/*.ts src/*/*.ts
sed -i~ "s/it('\[${DEVTOOLS_SKIP}\]/it.skip('/" tests/devtools.test.tsx
env:
DEVTOOLS_SKIP: ${{ matrix.devtools-skip }}
Comment on lines -72 to -73
Copy link
Member

Choose a reason for hiding this comment

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

If we are removing this, we can remove devtools-skip entirely. Can you try that? devtools-skip is a hack to pass CIs, so if it passes, it's fine.

Find also CI-MATRIX in ./tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I made that change to the best of my ability. I would appreciate a review of these changes because I'm not very familiar with the previous hacks in the CI although I understand why they were needed.

- name: Test ${{ matrix.react }} ${{ matrix.devtools-skip }}
run: |
yarn add -D react@${{ matrix.react }} react-dom@${{ matrix.react }}
Expand Down
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,8 +255,8 @@
},
"peerDependencies": {
"immer": ">=9.0",
"@types/react": ">=16.8",
"react": ">=16.8"
"@types/react": ">=18.0",
"react": ">=18.0"
},
"peerDependenciesMeta": {
"immer": {
Expand Down
Loading