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 all 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
32 changes: 3 additions & 29 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,24 +12,8 @@ jobs:
strategy:
fail-fast: false
matrix:
build: [cjs, umd] # [cjs, esm, umd, system]
env: [development, production]
devtools-skip:
- CI-MATRIX-NOSKIP
include:
- devtools-skip: CI-MATRIX-[2345]
build: umd
- devtools-skip: CI-MATRIX-[1345]
build: umd
- devtools-skip: CI-MATRIX-[1245]
build: umd
- devtools-skip: CI-MATRIX-[1235]
build: umd
- devtools-skip: CI-MATRIX-[1234]
build: umd
exclude:
- devtools-skip: CI-MATRIX-NOSKIP
build: umd
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]

steps:
- uses: actions/checkout@v2
- uses: actions/setup-node@v2
Expand All @@ -39,10 +23,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 react@17.0.2 react-dom@17.0.2 @testing-library/react@12.1.4
- name: Patch for DEV-ONLY
if: ${{ matrix.env == 'development' }}
run: |
Expand All @@ -60,24 +40,18 @@ 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 }}
- name: Patch for UMD (DEV)
if: ${{ matrix.build == 'umd' && matrix.env == 'development' }}
run: |
sed -i~ "s/<rootDir>\/src\(.*\)\.ts/<rootDir>\/dist\/umd\1.development.js/" package.json
sed -i~ "s/it('\[${DEVTOOLS_SKIP}\]/it.skip('/" tests/devtools.test.tsx
env:
DEVTOOLS_SKIP: ${{ matrix.devtools-skip }}
- name: Patch for UMD (PRD)
if: ${{ matrix.build == 'umd' && matrix.env == 'production' }}
run: |
sed -i~ "s/<rootDir>\/src\(.*\)\.ts/<rootDir>\/dist\/umd\1.production.js/" package.json
sed -i~ "s/it('\[${DEVTOOLS_SKIP}\]/it.skip('/" tests/devtools.test.tsx
env:
DEVTOOLS_SKIP: ${{ matrix.devtools-skip }}
- name: Patch for SystemJS
if: ${{ matrix.build == 'system' }}
run: |
Expand Down
32 changes: 1 addition & 31 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,29 +28,11 @@ jobs:
fail-fast: false
matrix:
react:
- 16.8.0
- 17.0.0
- 18.0.0
- 18.1.0
- 18.2.0
- 18.3.0-canary-6c7b41da3-20231123
- 0.0.0-experimental-6c7b41da3-20231123
devtools-skip:
- CI-MATRIX-NOSKIP
include:
- devtools-skip: CI-MATRIX-[2345]
react: 16.8.0
- devtools-skip: CI-MATRIX-[1345]
react: 16.8.0
- devtools-skip: CI-MATRIX-[1245]
react: 16.8.0
- devtools-skip: CI-MATRIX-[1235]
react: 16.8.0
- devtools-skip: CI-MATRIX-[1234]
react: 16.8.0
exclude:
- devtools-skip: CI-MATRIX-NOSKIP
react: 16.8.0
steps:
- uses: actions/checkout@v2
- uses: actions/setup-node@v2
Expand All @@ -59,19 +41,7 @@ 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/react@12.1.4
- 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 }}
- name: Test ${{ matrix.react }}
run: |
yarn add -D react@${{ matrix.react }} react-dom@${{ matrix.react }}
yarn test:ci
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 @@ -239,9 +239,9 @@
"vitest": "^0.34.6"
},
"peerDependencies": {
"@types/react": ">=16.8",
"@types/react": ">=18.0",
"immer": ">=9.0.6",
"react": ">=16.8"
"react": ">=18.0"
},
"peerDependenciesMeta": {
"@types/react": {
Expand Down
22 changes: 6 additions & 16 deletions tests/devtools.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -678,7 +678,7 @@ it('preserves isRecording after setting from devtools', async () => {
/* features:
* [] if name is undefined - use multiple devtools connections.
* [] if name and store is defined - use connection for specific 'name'.
* [] if two stores are coonected to one 'name' group and.
* [] if two stores are connected to one 'name' group and.
* another connected to another 'name' group, then feature should work
* [] check actions with this feature, for multiple stores that store prefixes are added -
* [] - reset
Expand Down Expand Up @@ -720,9 +720,7 @@ describe('when redux connection was called on multiple stores with `name` undefi
})

describe('when `store` property was provided in `devtools` call in options', () => {
// FIXME: Run this test separately in CI, until we're able to test modules in isolation i.e. use jest.resetModule and re-import modules in each test
// Relevant issues https://github.com/nodejs/node/issues/35889
it('[CI-MATRIX-1] should create single connection for all indernal calls of .connect and `store` is not passed to .connect', async () => {
it('should create single connection for all internal calls of .connect and `store` is not passed to .connect', async () => {
const { devtools: newDevtools } = await import('zustand/middleware')

const options1 = { store: 'store1123', foo: 'bar1' }
Expand All @@ -743,9 +741,7 @@ describe('when redux connection was called on multiple stores with `name` undefi
})
})

// FIXME: Run this test separately in CI, until we're able to test modules in isolation i.e. use jest.resetModule and re-import modules in each test
// Relevant issues https://github.com/nodejs/node/issues/35889
it('[CI-MATRIX-2] should call `.init` on single connection with combined states after each `create(devtools` call', async () => {
it('should call `.init` on single connection with combined states after each `create(devtools` call', async () => {
const { devtools: newDevtools } = await import('zustand/middleware')

const options1 = { store: 'store12' }
Expand Down Expand Up @@ -839,9 +835,7 @@ describe('when redux connection was called on multiple stores with `name` provid
})
})

// FIXME: Run this test separately in CI, until we're able to test modules in isolation i.e. use jest.resetModule and re-import modules in each test
// Relevant issues https://github.com/nodejs/node/issues/35889
it('[CI-MATRIX-3] should call `.init` on single connection with combined states after each `create(devtools` call', async () => {
it('should call `.init` on single connection with combined states after each `create(devtools` call', async () => {
const { devtools: newDevtools } = await import('zustand/middleware')
const connectionNameGroup1 = 'test1'
const connectionNameGroup2 = 'test2'
Expand Down Expand Up @@ -2352,9 +2346,7 @@ describe('when create devtools was called multiple times with `name` and `store`
console.error = originalConsoleError
})

// FIXME: Run this test separately in CI, until we're able to test modules in isolation i.e. use jest.resetModule and re-import modules in each test
// Relevant issues https://github.com/nodejs/node/issues/35889
it('[CI-MATRIX-4] does nothing even if there is `api.dispatch`, connections isolated from each other', async () => {
it('does nothing even if there is `api.dispatch`, connections isolated from each other', async () => {
const { devtools: newDevtools } = await import('zustand/middleware')

const name1 = 'name1'
Expand Down Expand Up @@ -2402,9 +2394,7 @@ describe('when create devtools was called multiple times with `name` and `store`
expect((api2 as any).dispatch).not.toBeCalled()
})

// FIXME: Run this test separately in CI, until we're able to test modules in isolation i.e. use jest.resetModule and re-import modules in each test
// Relevant issues https://github.com/nodejs/node/issues/35889
it('[CI-MATRIX-5] dispatches with `api.dispatch` when `api.dispatchFromDevtools` is set to true, connections are isolated from each other', async () => {
it('dispatches with `api.dispatch` when `api.dispatchFromDevtools` is set to true, connections are isolated from each other', async () => {
const { devtools: newDevtools } = await import('zustand/middleware')
const name1 = 'name1'
const name2 = 'name2'
Expand Down
Loading
Loading