Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR modernizes the codebase: migrates tests from Enzyme/Sinon to React Testing Library/Jest, replaces React.createFactory with createElement, updates lifecycle usages and context handling to use React Context + a context store, tightens observable callbacks, and upgrades build/test tooling and dependencies for modern React versions. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant WithContext as withContext HOC
participant ContextStore as Context Store
participant Provider as Context Provider
participant Base as BaseComponent
App->>WithContext: wrap BaseComponent + childContextTypes
WithContext->>ContextStore: getOrCreateContext(key) for each key
ContextStore-->>WithContext: return cached/new Context
App->>WithContext: render with props
WithContext->>Provider: for each key, wrap Base with Provider(value from getChildContext)
Provider->>Base: render BaseComponent with provided context values
Base-->>App: render output (uses context)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@claude Review. |
|
@coderabbitai Review. |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @.prettierrc:
- Line 4: Replace the deprecated parser setting in .prettierrc: change the
parser value from "babylon" to "babel" so Prettier uses the current parser name;
update the parser field in the .prettierrc file (the "parser" key) to "babel" to
avoid failures with Prettier >=2.0.
In `@package.json`:
- Line 85: Replace the outdated dependency "rollup-plugin-size-snapshot" in
package.json with a Rollup 4–compatible alternative (or remove it): remove the
"rollup-plugin-size-snapshot": "^0.12.0" entry, add a maintained plugin that
supports rollup@^4 (for example "rollup-plugin-filesize" or
"rollup-plugin-visualizer"), run your package manager to install, and update any
references in your Rollup config (e.g., import/usage of the old plugin in
rollup.config.js/rollup.config.mjs) to the new plugin's import name and API.
In `@src/packages/recompose/__tests__/componentFromStream-test.js`:
- Around line 91-106: The test fails because the vdom$ subscription created by
componentFromStream (using props$ and the first() operator) lacks an error
handler so unmounting completes props$ and causes an unhandled observable error;
fix by adding an error handler to the vdom$ subscription created inside
componentFromStream (or in the test subscribe mock) so it consumes the "no
elements in sequence" error, e.g., ensure the subscription created for vdom$
includes an onError callback, or alter the test assertion to not expect a
synchronous throw from unmount() (adjust the expect(() =>
unmount()).toThrowError(...) to the React 19 behavior such as asserting that no
uncaught exception escapes or using a spy on console/reportError).
In `@src/packages/recompose/__tests__/nest-test.js`:
- Around line 14-19: The lint rule flags that the forEach callback in the test
uses an arrow expression that implicitly returns the expect() result; update the
callback for divs.forEach to use a block-bodied function (e.g., divs.forEach(div
=> { expect(div.getAttribute('pass')).toBe('through') })) so it does not return
a value; locate the usage of the divs variable and the forEach call in the Nest
test in src/packages/recompose/__tests__/nest-test.js and replace the
implicit-return arrow with a block body to satisfy useIterableCallbackReturn.
In `@src/packages/recompose/__tests__/withState-test.js`:
- Around line 43-59: The test "withState accepts setState() callback" currently
embeds an assertion inside the callback mock but never asserts the callback was
actually invoked; update the test that uses
withState('counter','updateCounter',0) and the local callback to assert
invocation after calling updateCounter — e.g., after act(() => {
updateCounter(18, callback) }) add an explicit assertion such as
expect(callback).toHaveBeenCalled() or expect(callback).toHaveBeenCalledTimes(1)
to ensure the callback was executed.
In `@src/packages/recompose/utils/contextStore.js`:
- Around line 3-12: The module-level store currently uses a plain object (const
store = {}) which can collide with Object.prototype keys (e.g., "constructor" or
"toString") causing getOrCreateContext to return inherited properties; change
the store to a safe map-like container (e.g., replace the object with
Object.create(null) or a Map) and update getOrCreateContext accordingly (use
store[key] / store.has(key) / store.set(key, ...) or Map.get/Map.set/Map.has) so
context keys cannot collide with prototype properties while preserving
displayName logic in getOrCreateContext.
🧹 Nitpick comments (6)
src/packages/recompose/baconObservableConfig.js (1)
17-21: Guard ordering is functionally correct but subtly changes fall-through semantics.Placing the observer-method check before the event-type check means a value event with no
observer.nextwill fall through to evaluateobserver.error && event.isError()andobserver.complete && event.isEnd(). This is safe because Bacon event types are mutually exclusive, so no wrong branch can match. However, checking event type first (event.hasValue() && observer.next) would short-circuit earlier and make the intent clearer.♻️ Optional: check event type first
- if (observer.next && event.hasValue()) { + if (event.hasValue() && observer.next) { observer.next(event.value()) - } else if (observer.error && event.isError()) { + } else if (event.isError() && observer.error) { observer.error(event.error) - } else if (observer.complete && event.isEnd()) { + } else if (event.isEnd() && observer.complete) { observer.complete()src/packages/recompose/fromRenderProps.js (1)
1-1: Minor style inconsistency: usesReact.createElementinstead of destructuredcreateElement.Other migrated files (e.g.,
withState.js,lifecycle.js,mapPropsStream.js) destructurecreateElementfrom the import. This file uses the defaultReactimport and callsReact.createElement. Consider aligning for consistency.Proposed diff
-import React from 'react' +import { createElement } from 'react' import setDisplayName from './setDisplayName' import wrapDisplayName from './wrapDisplayName' @@ -11,10 +11,10 @@ const FromRenderProps = ownerProps => - React.createElement(RenderPropsComponent, { + createElement(RenderPropsComponent, { [renderPropName]: (...props) => - React.createElement(BaseComponent, { + createElement(BaseComponent, { ...ownerProps, ...propsMapper(...props), }),Also applies to: 11-16
src/packages/recompose/defaultProps.js (1)
6-13: Consider handlingownerProps.childrenpropagation explicitly for clarity.
createElement(BaseComponent, props)will treatprops.childrenas the children, which works correctly here sinceownerProps.childrenflows through the merge. However, if a consumer ever sets a default forchildrenin thedefaultsobject, the merging semantics may surprise — the defaultchildrenwould be replaced by the owner'schildrenonly if it's notundefined. This is a niche edge case and the current behavior is technically correct, just worth noting.src/packages/recompose/getContext.js (1)
10-18: Consider: each context key adds a nesting layer of Consumers at render time.If
contextTypeshas many keys, this creates a correspondingly deep component tree. For typical usage (a few keys) this is fine, but worth being aware of for heavily-used HOCs.src/packages/recompose/__tests__/branch-test.js (1)
51-59: Prefer idiomatic Jest assertion forconsole.error.Line 59 can be simplified for readability.
Proposed fix
- expect(console.error.mock.calls.length > 0).toBe(false) + expect(console.error).not.toHaveBeenCalled()src/packages/recompose/__tests__/toClass-test.js (1)
36-36: VestigialcontextTypesassignment — no longer has any effect.With the migration away from legacy context, setting
contextTypeson a function component is a no-op. Consider removing it to avoid confusion about how context is consumed in the updated codebase.Suggested diff
StatelessComponent.displayName = 'Stateless' StatelessComponent.propTypes = { foo: PropTypes.string } - StatelessComponent.contextTypes = { bar: PropTypes.object } StatelessComponent.defaultProps = { foo: 'bar', fizz: 'buzz' }
| "@rollup/plugin-replace": "^6.0.0", | ||
| "@rollup/plugin-terser": "^0.4.0", | ||
| "rollup": "^4.0.0", | ||
| "rollup-plugin-size-snapshot": "^0.12.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check what Rollup version is in package.json
cat package.json | grep -A2 -B2 '"rollup"'Repository: shakacode/recompose
Length of output: 231
🏁 Script executed:
# Check the package.json for rollup version
cat package.json | jq '.dependencies.rollup // .devDependencies.rollup // "not found"'Repository: shakacode/recompose
Length of output: 71
🏁 Script executed:
# Query npm registry for rollup-plugin-size-snapshot package info
npm view rollup-plugin-size-snapshot versions --json 2>/dev/null | tail -5Repository: shakacode/recompose
Length of output: 110
🏁 Script executed:
# Get recent releases and maintenance info
npm view rollup-plugin-size-snapshot --json 2>/dev/null | jq '{name, "dist-tags": .["dist-tags"], latest_version: .version, modified: .time.modified, maintainers: .maintainers}'Repository: shakacode/recompose
Length of output: 283
🏁 Script executed:
# Check specifically for rollup-plugin-size-snapshot@0.12.0 details
npm view rollup-plugin-size-snapshot@0.12.0 --json 2>/dev/null | jq '{version, peerDependencies, dependencies}'Repository: shakacode/recompose
Length of output: 405
Replace rollup-plugin-size-snapshot@^0.12.0 with a Rollup 4-compatible alternative.
rollup-plugin-size-snapshot@0.12.0 declares a peer dependency on rollup@^2.0.0, but your project uses rollup@^4.0.0. Additionally, the plugin has not been maintained since May 2022 and has no newer releases. This peer dependency mismatch can lead to runtime incompatibilities or unexpected behavior. Consider removing this plugin or migrating to a maintained alternative that supports Rollup 4.
🤖 Prompt for AI Agents
In `@package.json` at line 85, Replace the outdated dependency
"rollup-plugin-size-snapshot" in package.json with a Rollup 4–compatible
alternative (or remove it): remove the "rollup-plugin-size-snapshot": "^0.12.0"
entry, add a maintained plugin that supports rollup@^4 (for example
"rollup-plugin-filesize" or "rollup-plugin-visualizer"), run your package
manager to install, and update any references in your Rollup config (e.g.,
import/usage of the old plugin in rollup.config.js/rollup.config.mjs) to the new
plugin's import name and API.
There was a problem hiding this comment.
@alexeyr-ci2, understood! If you encounter any issues with the plugin in the future, feel free to revisit this.
|
@claude review this pr |
|
Claude finished @justin808's task in 2m 32s —— View job PR Review Complete ✅Reviewing PR #11: React 17-19 compatibility Tasks
Overall AssessmentThis is a well-executed modernization effort that successfully brings recompose to React 19 compatibility. The changes are comprehensive, properly tested, and address all the major breaking changes in React 17-19. CodeRabbit's review already caught several issues which were addressed in commit 23920d9. ✅ Strengths1. Excellent Test MigrationThe migration from Enzyme/Sinon to React Testing Library is comprehensive and well-done:
Example of good migration (from test('withStateHandlers handles events passed as argument', () => {
const component = jest.fn(() => null)
const InputComponent = withStateHandlers(
{ value: '' },
{ onChange: () => e => ({ value: e.target.value }) }
)(component)
render(<InputComponent />)
const { onChange } = component.mock.lastCall[0]
act(() => {
onChange({ target: { value: 'Yay' } })
})
expect(component.mock.lastCall[0].value).toBe('Yay')
})2. Smart Context API RewriteThe new context implementation using
const store = new Map()
export const getOrCreateContext = key => {
const existing = store.get(key)
if (existing) return existing
const ctx = createContext(undefined)
if (process.env.NODE_ENV !== 'production') {
ctx.displayName = key
}
store.set(key, ctx)
return ctx
}✅ Fixed issue from CodeRabbit: Originally used plain
3. Proper React.createFactory RemovalAll HOCs correctly migrated from
4. Observable Config Safety GuardsGood defensive programming in observable adapters:
if (observer.next && event.hasValue()) {
observer.next(event.value())
} else if (observer.error && event.isError()) {
observer.error(event.error)
} else if (observer.complete && event.isEnd()) {
observer.complete()
}This prevents errors when observers have partial callback implementations. 5. Clean defaultProps ImplementationThe new
const DefaultProps = ownerProps => {
const props = { ...defaults }
Object.keys(ownerProps).forEach(key => {
if (ownerProps[key] !== undefined || !(key in defaults)) {
props[key] = ownerProps[key]
}
})
return createElement(BaseComponent, props)
}Correctly handles undefined props by falling back to defaults.
|
Replace enzyme (no React 19 support) and sinon with @testing-library/react and jest built-in mocks. Update jest to v29, babel to ^7.22.0. Rewrite all 42 test files. Fix observable configs (bacon, rxjs4, flyd) to guard observer methods before calling them. Add restoreMocks to jest config. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
createFactory was deprecated in React 16.13 and removed in React 19. Replace all 17 source files and 1 test file that used it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
componentWillMount and componentWillReceiveProps were deprecated without the UNSAFE_ prefix in React 16.3 and will be removed in React 19. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace legacy context API (childContextTypes, getChildContext, contextTypes) with React.createContext via an internal context registry keyed by name. Remove contextTypes copying and this.context from toClass. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
React 19 no longer supports .defaultProps on function components. Replace with inline merging that preserves the same semantics: undefined owner props fall back to defaults. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
getDerivedStateFromProps is natively supported since React 16.3, so the polyfill is unnecessary. Event pooling was removed in React 17, making persist() a no-op. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- react, react-dom: ^16.3.1 → ^19.0.0 - @testing-library/react: ^11.2.7 → ^16.3.0 (add @testing-library/dom) - peerDependencies.react: ^17.0.0 || ^18.0.0 || ^19.0.0 - Remove console.error assertion in componentFromStream test (React 19 reports uncaught errors via window.reportError instead of console.error) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Rollup 0.65 → 4.x with @rollup/ scoped plugins - Replace rollup-plugin-uglify with @rollup/plugin-terser - Remove webpack (only used by old rollup-plugin-size-snapshot) - Prettier 1.2 → 1.13 to support import.meta syntax - eslint-plugin-prettier 2.x → 3.x to read .prettierrc automatically - Remove duplicated prettier options from format script and lint-staged - Remove sinon global from .eslintrc Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
alexeyr-ci2
force-pushed
the
alexeyr/react-19
branch
from
23920d9 to
0ccdede
Compare
|
@coderabbitai Review. |
|
@claude Review. |
|
✅ Actions performedReview triggered.
|
|
Claude finished @alexeyr-ci2's task in 2m 43s —— View job PR Review Complete ✅Reviewing PR #11: React 17-19 compatibility Tasks
Overall AssessmentThis is a well-executed modernization effort that successfully brings recompose to React 19 compatibility. The changes are comprehensive, properly tested, and address all the major breaking changes in React 17-19. CodeRabbit's review already caught several issues which were addressed in commit 0ccdede. ✅ Strengths1. Excellent Test MigrationThe migration from Enzyme/Sinon to React Testing Library is comprehensive and well-done:
Example of good migration (from test('withStateHandlers handles events passed as argument', () => {
const component = jest.fn(() => null)
const InputComponent = withStateHandlers(
{ value: '' },
{ onChange: () => e => ({ value: e.target.value }) }
)(component)
render(<InputComponent />)
const { onChange } = component.mock.lastCall[0]
act(() => {
onChange({ target: { value: 'Yay' } })
})
expect(component.mock.lastCall[0].value).toBe('Yay')
})2. Smart Context API RewriteThe new context implementation using
const store = new Map()
export const getOrCreateContext = key => {
const existing = store.get(key)
if (existing) return existing
const ctx = createContext(undefined)
if (process.env.NODE_ENV !== 'production') {
ctx.displayName = key
}
store.set(key, ctx)
return ctx
}✅ Fixed issue from CodeRabbit: Originally used plain
3. Proper React.createFactory RemovalAll HOCs correctly migrated from
4. Observable Config Safety GuardsGood defensive programming in observable adapters:
if (observer.next && event.hasValue()) {
observer.next(event.value())
} else if (observer.error && event.isError()) {
observer.error(event.error)
} else if (observer.complete && event.isEnd()) {
observer.complete()
}This prevents errors when observers have partial callback implementations. 5. Clean defaultProps ImplementationThe new
const DefaultProps = ownerProps => {
const props = { ...defaults }
Object.keys(ownerProps).forEach(key => {
if (ownerProps[key] !== undefined || !(key in defaults)) {
props[key] = ownerProps[key]
}
})
return createElement(BaseComponent, props)
}Correctly handles undefined props by falling back to defaults.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 76-77: Tests are only running against "react" and "react-dom"
pinned to ^19.0.0 while package peerDependencies declare "react": "^17.0.0 ||
^18.0.0 || ^19.0.0", so add CI jobs to validate those claims: update your CI
workflow to include a matrix over REACT_VERSION (17 18 19), install matching
versions of "react" and "react-dom" for each matrix entry before running the
test script, or alternately adjust devDependencies to install multiple versions
via npm aliases in the CI run; ensure the CI uses the peerDependency string
"react": "^17.0.0 || ^18.0.0 || ^19.0.0" as the source of truth and that the
test command is executed for each matrix entry to verify compatibility.
🧹 Nitpick comments (4)
src/packages/recompose/__tests__/withStateHandlers-test.js (1)
82-90: Spy cleanup needed; assertion could be more idiomatic.The
jest.spyOn(console, 'error')mock is never restored. The Jest config has"restoreMocks": true(which only restoresjest.mock()calls, not spies), so the spy persists across test execution.Additionally,
expect(console.error.mock.calls.length > 0).toBe(true)should be replaced with the more idiomaticexpect(errorSpy).toHaveBeenCalled().Suggested fix
test('withStateHandlers initial state must be function or object or null or undefined', () => { const component = jest.fn(() => null) component.displayName = 'component' const Counter = withStateHandlers(1, {})(component) - jest.spyOn(console, 'error').mockImplementation(() => {}) + const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}) render(<Counter />) - expect(console.error.mock.calls.length > 0).toBe(true) + expect(errorSpy).toHaveBeenCalled() + errorSpy.mockRestore() })src/packages/recompose/__tests__/withHandlers-test.js (1)
70-72: Stale comment references "React 16" but PR targets React 17-19.The comment on line 70 mentions "React 16 dev mode" but this PR drops React 16 support. The error-rethrowing behavior persists in later versions too, so the suppression technique remains correct—just the comment text is outdated.
✏️ Suggested comment update
- // React 16 dev mode re-throws errors from event handlers via - // invokeGuardedCallbackDev, causing an uncaught error in jsdom. + // React dev mode re-throws errors from event handlers via + // invokeGuardedCallbackDev, causing an uncaught error in jsdom.src/packages/recompose/__tests__/branch-test.js (1)
63-63: Prefer idiomatic Jest matcher for readability.
expect(console.error.mock.calls.length > 0).toBe(false)works but is less expressive than the built-in matcher.✏️ Suggested change
- expect(console.error.mock.calls.length > 0).toBe(false) + expect(console.error).not.toHaveBeenCalled()package.json (1)
42-48: Replace@babel/plugin-proposal-class-propertieswith@babel/plugin-transform-class-properties.The proposal plugin is deprecated because class fields are now part of the ECMAScript standard. The maintained replacement is
@babel/plugin-transform-class-properties. The old package still works but shows npm deprecation warnings. If you useloose: trueconfiguration, apply the same option to the transform plugin.
| "react": "^19.0.0", | ||
| "react-dom": "^19.0.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the package.json file to check peerDependencies
find . -name "package.json" -type f | head -5Repository: shakacode/recompose
Length of output: 257
🏁 Script executed:
# Show the full package.json to see peerDependencies section
cat package.json | jq '{peerDependencies, devDependencies: .devDependencies | with_entries(select(.key == "react" or .key == "react-dom"))}'Repository: shakacode/recompose
Length of output: 172
🏁 Script executed:
# Search for CI configuration files that might specify test matrix
find . -type f \( -name "*.yml" -o -name "*.yaml" \) | grep -E "(workflow|ci|github)" | head -10Repository: shakacode/recompose
Length of output: 634
🏁 Script executed:
# Check full peerDependencies from root package.json
jq '.peerDependencies' package.jsonRepository: shakacode/recompose
Length of output: 67
🏁 Script executed:
# Check the recompose package's own package.json for peerDependencies
cat src/packages/recompose/package.json | jq '{peerDependencies, devDependencies: .devDependencies | with_entries(select(.key == "react" or .key == "react-dom"))}'Repository: shakacode/recompose
Length of output: 113
🏁 Script executed:
# Examine the CI workflow files to check for test matrix
cat .github/workflows/claude-code-review.yml | head -50Repository: shakacode/recompose
Length of output: 1495
🏁 Script executed:
# Also check the other workflow
cat .github/workflows/claude.yml | head -50Repository: shakacode/recompose
Length of output: 1948
🏁 Script executed:
# Check the recompose package's package.json directly
cat src/packages/recompose/package.json | head -40Repository: shakacode/recompose
Length of output: 645
🏁 Script executed:
# Search for any peerDependencies mentions in the codebase
rg "peerDependencies" --type jsonRepository: shakacode/recompose
Length of output: 188
🏁 Script executed:
# Check if there are any other test workflow files that might have a matrix
find . -path ./node_modules -prune -o -type f -name "*.yml" -o -name "*.yaml" | xargs grep -l "react\|matrix" 2>/dev/null | grep -v node_modules | head -10Repository: shakacode/recompose
Length of output: 45
🏁 Script executed:
# Check what the root package.json test script does
cat package.json | jq '.scripts.test'Repository: shakacode/recompose
Length of output: 135
🏁 Script executed:
# Look at the jest configuration to understand what's being tested
cat package.json | jq '.jest'Repository: shakacode/recompose
Length of output: 272
🏁 Script executed:
# Check if there's a .github/workflows/test.yml or similar that runs the actual tests
find .github/workflows -name "*.yml" -o -name "*.yaml" | xargs cat 2>/dev/null | grep -A 20 "test\|jest" | head -50Repository: shakacode/recompose
Length of output: 1387
🏁 Script executed:
# Check the structure - is there a monorepo setup?
ls -la src/packages/Repository: shakacode/recompose
Length of output: 359
Tests only run against React 19, but the library claims React 17/18 support.
The recompose package declares "react": "^17.0.0 || ^18.0.0 || ^19.0.0" as a peer dependency, yet the root package.json pins react and react-dom devDependencies to ^19.0.0 only. The test suite will validate against React 19 exclusively. Consider adding a CI matrix or local verification step to run tests against React 17 and 18 to ensure compatibility claims are backed by actual validation.
🤖 Prompt for AI Agents
In `@package.json` around lines 76 - 77, Tests are only running against "react"
and "react-dom" pinned to ^19.0.0 while package peerDependencies declare
"react": "^17.0.0 || ^18.0.0 || ^19.0.0", so add CI jobs to validate those
claims: update your CI workflow to include a matrix over REACT_VERSION (17 18
19), install matching versions of "react" and "react-dom" for each matrix entry
before running the test script, or alternately adjust devDependencies to install
multiple versions via npm aliases in the CI run; ensure the CI uses the
peerDependency string "react": "^17.0.0 || ^18.0.0 || ^19.0.0" as the source of
truth and that the test command is executed for each matrix entry to verify
compatibility.
There was a problem hiding this comment.
The APIs used are stable. Adding tests for older React versions is a potential improvement, but not worth the complexity.
There was a problem hiding this comment.
@alexeyr-ci2, understood. I've noted your preference for the future.
- Use Map in contextStore to avoid prototype key collisions - Use block body in forEach callback in nest-test - Assert setState callback was invoked in withState-test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
alexeyr-ci2
force-pushed
the
alexeyr/react-19
branch
from
0ccdede to
a21ea4e
Compare
|
@coderabbitai Review. |
|
@claude Review. |
|
✅ Actions performedReview triggered.
|
|
Claude finished @alexeyr-ci2's task in 2m 18s —— View job PR Review Complete ✅Reviewing PR #11: React 17-19 compatibility Tasks
Overall AssessmentThis is a well-executed modernization effort that successfully brings recompose to React 19 compatibility. The PR addresses all major breaking changes in React 17-19 comprehensively, with excellent test coverage. The latest commit (a21ea4e) addressed the key issues raised by CodeRabbit's previous review. ✅ Strengths1. Excellent Test MigrationThe migration from Enzyme/Sinon to React Testing Library is comprehensive and properly executed:
Example of good migration ( test('withState accepts setState() callback', () => {
const component = jest.fn(() => null)
const Counter = withState('counter', 'updateCounter', 0)(component)
render(<Counter />)
const { updateCounter } = component.mock.calls[0][0]
const callback = jest.fn(() => {
expect(component.mock.lastCall[0].counter).toBe(18)
})
act(() => {
updateCounter(18, callback)
})
expect(callback).toHaveBeenCalledTimes(1) // ✅ Properly verifies callback invocation
})2. Smart Context API RewriteThe new context implementation using
const store = new Map() // ✅ Using Map prevents prototype pollution
export const getOrCreateContext = key => {
const existing = store.get(key)
if (existing) return existing
const ctx = createContext(undefined)
if (process.env.NODE_ENV !== 'production') {
ctx.displayName = key
}
store.set(key, ctx)
return ctx
}
const WithContext = props => {
const contextValues = getChildContext(props)
let element = createElement(BaseComponent, props)
contextKeys.forEach(key => {
const Ctx = getOrCreateContext(key)
element = createElement(
Ctx.Provider,
{ value: contextValues[key] },
element
)
})
return element
}This approach correctly wraps the component in nested Providers, maintaining the legacy API signature while using modern Context API internally. 3. Proper React.createFactory RemovalAll HOCs correctly migrated from
4. Observable Config Safety GuardsGood defensive programming in observable adapters:
if (observer.next && event.hasValue()) {
observer.next(event.value())
} else if (observer.error && event.isError()) {
observer.error(event.error)
} else if (observer.complete && event.isEnd()) {
observer.complete()
}This prevents errors when observers have partial callback implementations. 5. Clean defaultProps ImplementationThe new
const DefaultProps = ownerProps => {
const props = { ...defaults }
Object.keys(ownerProps).forEach(key => {
if (ownerProps[key] !== undefined || !(key in defaults)) {
props[key] = ownerProps[key]
}
})
return createElement(BaseComponent, props)
}Correctly handles undefined props by falling back to defaults. 6. CodeRabbit Issues AddressedThe latest commit (a21ea4e) properly addressed CodeRabbit's feedback:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@scripts/rollup.config.js`:
- Line 8: The import of the unmaintained rollup-plugin-size-snapshot (symbol
sizeSnapshot in rollup.config.js) is incompatible with Rollup 4; remove the
sizeSnapshot import and any usage of the sizeSnapshot plugin in the Rollup
config and replace it with a maintained alternative (e.g.,
rollup-plugin-filesize or rollup-plugin-visualizer): install the chosen package,
import its symbol in place of sizeSnapshot, and wire that plugin into the
plugins array where sizeSnapshot was used so you avoid forcing peer dependencies
on Rollup 2.
In `@src/packages/recompose/__tests__/withStateHandlers-test.js`:
- Around line 87-89: The test spies on console.error via jest.spyOn(console,
'error').mockImplementation but never restores it, causing a leaked mock; update
the test around render(<Counter />) and the
expect(console.error).toHaveBeenCalled() assertion to restore the spy after the
assertion (e.g., call console.error.mockRestore() or use a try/finally to ensure
mockRestore() runs), or move the spy/restore into an afterEach so the
jest.spyOn(console, 'error') created in this test is always restored; refer to
the jest.spyOn(console, 'error').mockImplementation, render, and Counter symbols
when making the change.
In `@src/packages/recompose/getContext.js`:
- Around line 6-7: The current getContext implementation discards the PropTypes
validators by only using Object.keys(contextTypes); restore the old behavior by
attaching the original validators to the wrapped component so PropTypes warnings
still occur: inside getContext (the function that returns a wrapper for
BaseComponent) copy/assign the incoming contextTypes object to the resulting
wrapper (e.g., set WrappedComponent.contextTypes = contextTypes) and preserve
any existing metadata like displayName; alternatively, if you intentionally
remove PropTypes, add a concise comment and migration note near getContext
explaining that validators are dropped.
🧹 Nitpick comments (4)
src/packages/recompose/__tests__/setObservableConfig-test.js (1)
22-55: Optional: reduce duplication withtest.each.All seven test cases share identical transform logic — only the config and label differ. You could use Jest's
test.eachto consolidate:♻️ Suggested refactor
-test('works with RxJS 5', () => { - setObservableConfig(rxjs5Config) - testTransform(props$ => props$.map(({ n }) => <div>{n * 2}</div>)) -}) - -test('works with RxJS 4', () => { - setObservableConfig(rxjs4Config) - testTransform(props$ => props$.map(({ n }) => <div>{n * 2}</div>)) -}) - -test('works with most', () => { - setObservableConfig(mostConfig) - testTransform(props$ => props$.map(({ n }) => <div>{n * 2}</div>)) -}) - -test('works with xstream', () => { - setObservableConfig(xstreamConfig) - testTransform(props$ => props$.map(({ n }) => <div>{n * 2}</div>)) -}) - -test('works with bacon', () => { - setObservableConfig(baconConfig) - testTransform(props$ => props$.map(({ n }) => <div>{n * 2}</div>)) -}) - -test('works with kefir', () => { - setObservableConfig(kefirConfig) - testTransform(props$ => props$.map(({ n }) => <div>{n * 2}</div>)) -}) - -test('works with flyd', () => { - setObservableConfig(flydConfig) - testTransform(props$ => props$.map(({ n }) => <div>{n * 2}</div>)) -}) +test.each([ + ['RxJS 5', rxjs5Config], + ['RxJS 4', rxjs4Config], + ['most', mostConfig], + ['xstream', xstreamConfig], + ['bacon', baconConfig], + ['kefir', kefirConfig], + ['flyd', flydConfig], +])('works with %s', (_, config) => { + setObservableConfig(config) + testTransform(props$ => props$.map(({ n }) => <div>{n * 2}</div>)) +})src/packages/recompose/__tests__/withStateHandlers-test.js (1)
124-154: Test "passes immutable state updaters" is identical to "have access to props" and doesn't verify immutability.Both tests have the same setup, the same single
increment()call, and the same assertion. The original Enzyme-based test likely verified that the updater function reference was stable across re-renders. After the RTL migration, that distinguishing behavior is no longer tested. Consider either removing this duplicate or adding an assertion that theincrementreference from the first render is the same object after a re-render:const incrementBefore = component.mock.calls[0][0].increment act(() => { incrementBefore() }) const incrementAfter = component.mock.lastCall[0].increment expect(incrementAfter).toBe(incrementBefore)src/packages/recompose/__tests__/toClass-test.js (1)
31-44: Line 36:contextTypesassignment is dead code.
StatelessComponent.contextTypesis set but never asserted or used in this test. Since legacy context was removed in React 19, consider removing this line to avoid confusion.Proposed cleanup
StatelessComponent.displayName = 'Stateless' StatelessComponent.propTypes = { foo: PropTypes.string } - StatelessComponent.contextTypes = { bar: PropTypes.object } StatelessComponent.defaultProps = { foo: 'bar', fizz: 'buzz' }src/packages/recompose/__tests__/fromRenderProps-test.js (1)
15-16:innerHTMLassertions are order-sensitive.Using
container.innerHTMLto assert rendered HTML works but is brittle — if React ever changes attribute serialization order, these tests break. Consider usinggetAttributechecks (as done indefaultProps-test.js) for more resilient assertions. This is a minor concern since React's attribute order is deterministic in practice.Also applies to: 31-32, 55-56, 72-73, 88-89
| import replace from '@rollup/plugin-replace' | ||
| import commonjs from '@rollup/plugin-commonjs' | ||
| import terser from '@rollup/plugin-terser' | ||
| import { sizeSnapshot } from 'rollup-plugin-size-snapshot' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat package.json | jq '.devDependencies, .dependencies' | grep -A 5 -B 5 "rollup"Repository: shakacode/recompose
Length of output: 538
🌐 Web query:
rollup-plugin-size-snapshot Rollup 4 compatibility
💡 Result:
rollup-plugin-size-snapshot is not officially compatible with Rollup 4.
- The latest release (
0.12.0, May 2020) declarespeerDependencies: { "rollup": "^2.0.0" }, so package managers will treat Rollup 3/4 as unsupported unless you force/override peer deps. [1] - The project also appears inactive (no releases in ~5 years), which makes Rollup 4 support unlikely without a fork/patch. [2]
If you must try it on Rollup 4, you’d be in “best-effort” territory: override peer deps and run your build/tests, but expect potential breakage. [1]
Sources: [1] [2]
rollup-plugin-size-snapshot is incompatible with Rollup 4.
Version 0.12.0 (the latest, from May 2020) declares peerDependencies: { "rollup": "^2.0.0" }, restricting it to Rollup 2 only. The project is unmaintained with no releases for ~5 years. Using it with Rollup 4 requires forcing peer dependencies, which is fragile and unsupported. Consider switching to an active alternative, maintaining a fork, or downgrading Rollup.
🤖 Prompt for AI Agents
In `@scripts/rollup.config.js` at line 8, The import of the unmaintained
rollup-plugin-size-snapshot (symbol sizeSnapshot in rollup.config.js) is
incompatible with Rollup 4; remove the sizeSnapshot import and any usage of the
sizeSnapshot plugin in the Rollup config and replace it with a maintained
alternative (e.g., rollup-plugin-filesize or rollup-plugin-visualizer): install
the chosen package, import its symbol in place of sizeSnapshot, and wire that
plugin into the plugins array where sizeSnapshot was used so you avoid forcing
peer dependencies on Rollup 2.
| jest.spyOn(console, 'error').mockImplementation(() => {}) | ||
| render(<Counter />) | ||
| expect(console.error).toHaveBeenCalled() |
There was a problem hiding this comment.
console.error spy is never restored after the test.
If a later test depends on console.error not being mocked (or checks it independently), this leaking mock could cause flaky failures. Call mockRestore() after the assertion.
Proposed fix
jest.spyOn(console, 'error').mockImplementation(() => {})
render(<Counter />)
expect(console.error).toHaveBeenCalled()
+ console.error.mockRestore()
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| jest.spyOn(console, 'error').mockImplementation(() => {}) | |
| render(<Counter />) | |
| expect(console.error).toHaveBeenCalled() | |
| jest.spyOn(console, 'error').mockImplementation(() => {}) | |
| render(<Counter />) | |
| expect(console.error).toHaveBeenCalled() | |
| console.error.mockRestore() |
🤖 Prompt for AI Agents
In `@src/packages/recompose/__tests__/withStateHandlers-test.js` around lines 87 -
89, The test spies on console.error via jest.spyOn(console,
'error').mockImplementation but never restores it, causing a leaked mock; update
the test around render(<Counter />) and the
expect(console.error).toHaveBeenCalled() assertion to restore the spy after the
assertion (e.g., call console.error.mockRestore() or use a try/finally to ensure
mockRestore() runs), or move the spy/restore into an afterEach so the
jest.spyOn(console, 'error') created in this test is always restored; refer to
the jest.spyOn(console, 'error').mockImplementation, render, and Counter symbols
when making the change.
| const getContext = contextTypes => BaseComponent => { | ||
| const factory = createFactory(BaseComponent) | ||
| const GetContext = (ownerProps, context) => | ||
| factory({ | ||
| ...ownerProps, | ||
| ...context, | ||
| }) | ||
| const contextKeys = Object.keys(contextTypes) |
There was a problem hiding this comment.
contextTypes values (PropTypes validators) are silently ignored.
The old getContext used contextTypes both for keys and for PropTypes validation. The new implementation only reads Object.keys(contextTypes) and discards the validator values. This is a behavioral change — invalid context values will no longer trigger PropTypes warnings. This is likely acceptable since PropTypes themselves are deprecated, but it's worth a note in migration docs or a comment.
🤖 Prompt for AI Agents
In `@src/packages/recompose/getContext.js` around lines 6 - 7, The current
getContext implementation discards the PropTypes validators by only using
Object.keys(contextTypes); restore the old behavior by attaching the original
validators to the wrapped component so PropTypes warnings still occur: inside
getContext (the function that returns a wrapper for BaseComponent) copy/assign
the incoming contextTypes object to the resulting wrapper (e.g., set
WrappedComponent.contextTypes = contextTypes) and preserve any existing metadata
like displayName; alternatively, if you intentionally remove PropTypes, add a
concise comment and migration note near getContext explaining that validators
are dropped.
- Fix duplicate withStateHandlers test by verifying updater reference stability across re-renders instead of duplicating "have access to props" assertions - Remove dead contextTypes assignment in toClass test (legacy context removed in React 19) - Replace brittle innerHTML assertions in fromRenderProps tests with getAttribute checks - Add comment in getContext.js noting PropTypes validators are ignored - Update withContext/getContext TypeScript types and API docs to reflect that only contextTypes keys are used Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai Please check the current state. |
|
@claude Review. |
|
Claude finished @alexeyr-ci2's task in 2m 3s —— View job PR Review Complete ✅Reviewing PR #11: React 17-19 compatibility Tasks
Overall AssessmentThis is a well-executed modernization effort that successfully brings recompose to React 19 compatibility. The changes are comprehensive, properly tested, and address all the major breaking changes in React 17-19. CodeRabbit's review already caught several issues which were addressed in commit a21ea4e. ✅ Strengths1. Excellent Test MigrationThe migration from Enzyme/Sinon to React Testing Library is comprehensive and well-done:
Example of good migration (from test('withStateHandlers handles events passed as argument', () => {
const component = jest.fn(() => null)
const InputComponent = withStateHandlers(
{ value: '' },
{ onChange: () => e => ({ value: e.target.value }) }
)(component)
render(<InputComponent />)
const { onChange } = component.mock.lastCall[0]
act(() => {
onChange({ target: { value: 'Yay' } })
})
expect(component.mock.lastCall[0].value).toBe('Yay')
})2. Smart Context API RewriteThe new context implementation using
const store = new Map()
export const getOrCreateContext = key => {
const existing = store.get(key)
if (existing) return existing
const ctx = createContext(undefined)
if (process.env.NODE_ENV !== 'production') {
ctx.displayName = key
}
store.set(key, ctx)
return ctx
}✅ Fixed issue from CodeRabbit: Originally used plain
3. Proper React.createFactory RemovalAll HOCs correctly migrated from
4. Observable Config Safety GuardsGood defensive programming in observable adapters:
if (observer.next && event.hasValue()) {
observer.next(event.value())
} else if (observer.error && event.isError()) {
observer.error(event.error)
} else if (observer.complete && event.isEnd()) {
observer.complete()
}This prevents errors when observers have partial callback implementations. 5. Clean defaultProps ImplementationThe new
const DefaultProps = ownerProps => {
const props = { ...defaults }
Object.keys(ownerProps).forEach(key => {
if (ownerProps[key] !== undefined || !(key in defaults)) {
props[key] = ownerProps[key]
}
})
return createElement(BaseComponent, props)
}Correctly handles undefined props by falling back to defaults.
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 22528
Status Overview
CI/Check Status
Outstanding IssuesI've identified 3 actionable issues that need attention: 1. Rollup plugin compatibilityFile:
2. Test spy leakFile:
3. PropTypes validators droppedFile:
Additional Nitpicks (4)
Would you like me to help address any of these issues? |
|
@CodeRabbit Most of these issues are handled already.
Are all fixed, test simplification is out of scope for the PR. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1902
✅ Jest config has I apologize for the noise—all the issues I flagged have indeed been addressed. The PR looks good from my perspective now. Great work on the comprehensive React 17-19 migration! 🎉 |
3 participants
Summary
React.createFactorywithReact.createElement(removed in React 19)UNSAFE_prefix to deprecated lifecycle methodswithContext/getContextwithReact.createContext(legacy context removed in React 19)defaultPropsHOC for function components (.defaultPropsremoved in React 19)react-lifecycles-compatpolyfill andevent.persist()(no-ops since React 16.3/17)^17.0.0 || ^18.0.0 || ^19.0.0symbol-observableto^4.0.0to follow the description in README@rollup/scoped plugins, replace uglify with terserTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores