-
Notifications
You must be signed in to change notification settings - Fork 17
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
chore: add support for react v19 & fix types #10
Conversation
WalkthroughThe pull request introduces updates to the project's configuration and dependencies for the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
"@types/react": "^19.0.2", | ||
"@types/react-dom": "^19.0.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it said i need to explicitly add these
"rollup": "^3.26.2", | ||
"rollup-plugin-inject-process-env": "^1.3.1", | ||
"typescript": "^5.0.4", | ||
"wavesurfer.js": "^7.8.1" | ||
"wavesurfer.js": "^7.8.11" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i went ahead an updated this as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.eslintrc.json (1)
4-5
: Consider adding React-specific ESLint rulesWhile adding the Jest environment is good, consider also adding React-specific ESLint rules to catch potential issues with React 19 features.
Consider adding these rules to the "rules" section:
"rules": { + "react/jsx-uses-react": "off", + "react/react-in-jsx-scope": "off" }tests/index.test.js (2)
23-24
: Consider implementing destroy method behaviorWhile the empty destroy method satisfies the interface, consider implementing basic cleanup behavior to match WaveSurfer's actual destroy method.
Consider implementing basic cleanup:
- // eslint-disable-next-line @typescript-eslint/no-empty-function - MockWaveSurfer.prototype.destroy = function () {} + MockWaveSurfer.prototype.destroy = function () { + this.listeners = {} + }
Line range hint
31-44
: Update test assertions for React 19 compatibilityThe test assertions look good, but consider adding tests for React 19 specific features and cleanup behavior.
Consider adding a test case for cleanup:
it('should properly cleanup on unmount', () => { const { unmount } = render(React.createElement(WavesurferPlayer, { waveColor: 'purple' })) const mockInstance = WaveSurfer.create.mock.results[0].value const destroySpy = jest.spyOn(mockInstance, 'destroy') unmount() expect(destroySpy).toHaveBeenCalled() })src/index.tsx (1)
Line range hint
220-224
: Consider aligning null handling between hooks.While the
HTMLDivElement
type is correctly specified, there's an inconsistency in null handling betweenuseWavesurfer
anduseWavesurferInstance
. For consistency, consider updating the container type to match:-}: Omit<WaveSurferOptions, 'container'> & { container: RefObject<HTMLDivElement> }): ReturnType< +}: Omit<WaveSurferOptions, 'container'> & { container: RefObject<HTMLDivElement | null> }): ReturnType<
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (4)
.eslintrc.json
(2 hunks)package.json
(2 hunks)src/index.tsx
(3 hunks)tests/index.test.js
(1 hunks)
🔇 Additional comments (4)
package.json (2)
38-39
: Verify React 19 compatibility with other dependencies
The upgrade to React 19 is significant. While the peer dependency has been updated correctly, we should ensure all React-related dependencies are compatible with this major version.
✅ Verification successful
The previous results show that @testing-library dependencies are compatible with React 19. Let's check other dependencies and usage patterns to ensure full compatibility.
Let's examine the specific React imports and dev dependencies to ensure everything is properly configured for React 19.
React 19 upgrade is properly configured
The verification shows that:
- All React-related dependencies (@testing-library/react, @types/react, @types/react-dom) are compatible with React 19
- React imports in the codebase use standard APIs that are stable across versions
- Type definitions are correctly updated to version 19
- No deprecated or incompatible React patterns were found in the imports
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential React 19 compatibility issues in dependencies
npm info "@testing-library/react@16.1.0" peerDependencies
npm info "@testing-library/dom@10.4.0" peerDependencies
Length of output: 305
Script:
#!/bin/bash
# Check for other React-dependent packages in package.json
rg "\"react\":|\"react-" package.json -A 1
# Look for potential React imports in the codebase
rg "from ['|\"]react" -l
Length of output: 267
Script:
#!/bin/bash
# Check the actual React imports and usage in the files
rg "from ['|\"]react" -A 2 tests/index.test.js src/index.tsx examples/app.tsx
# Check package.json for any additional React-related dev dependencies
rg "\"@types/react|\"@testing-library/react" package.json -A 1
Length of output: 991
46-50
: Consider testing library version constraints
The significant jump in @testing-library/react version (14 -> 16) might introduce breaking changes. Additionally, ensure that @testing-library/dom version 10.4.0 is compatible with @testing-library/react 16.1.0.
src/index.tsx (2)
37-37
: LGTM! Type safety and null handling improvements.
The changes improve the code in two ways:
- More specific typing with
HTMLDivElement
better represents wavesurfer's container requirements - Proper null handling with optional chaining prevents potential runtime errors
Also applies to: 46-46
Line range hint 1-230
: Verify documentation and examples reflect type changes.
The type changes to HTMLDivElement
and null handling are significant enough to potentially affect consuming applications. Please ensure:
- Documentation comments (especially the example in the useWavesurfer hook) reflect these type changes
- Any example projects or documentation sites are updated
✅ Verification successful
Documentation and examples are consistent with type changes
The type changes are properly reflected across the codebase:
- The example in README.md shows the correct usage with
useRef(null)
, which is compatible with theHTMLDivElement | null
type - The example app (examples/app.tsx) doesn't use
useRef
directly but uses the component correctly - Tests are type-agnostic and use
HTMLElement
appropriately in expectations - The JSDoc example in the source code correctly shows
useRef<HTMLDivElement | null>(null)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if examples in documentation match the new types
rg -A 10 "useRef.*HTMLDivElement" README.md docs/
# Check for potentially affected type usage in examples
fd -e tsx -e ts . examples/ --exec rg -l "useRef.*HTMLElement"
Length of output: 162
Script:
#!/bin/bash
# Let's check for any documentation files in the repository
fd -e md
# Check for examples in the repository root and any subdirectories
rg -A 10 "useRef"
# Check for any TypeScript/JavaScript example files
fd -e ts -e tsx -e js -e jsx
Length of output: 2290
Script:
#!/bin/bash
# Let's check the example app implementation
cat examples/app.tsx
# Check if there are any type inconsistencies in tests
cat tests/index.test.js
Length of output: 3586
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks so much!
The problem
Now that React 19 is stable, we can bump the peer dependency for it.
The solution
This upgrades the
peerDependencies
for React to v19, so apps that have migrated to React 19 don't get an error when installing packages. I also updated thecontainerRef
because React's types were updated.I added the
jest
env to eslint so it wouldn't complain when looking at the test file 😅Summary by CodeRabbit
New Features
destroy
method in the test suite forWavesurferPlayer
.Dependency Updates
Bug Fixes
useWavesurferInstance
anduseWavesurfer
functions.