Fixed type mismatch #217
Fixed type mismatch #217
Conversation
🦋 Changeset detectedLatest commit: 027fcd4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe changes in this pull request introduce a patch for the Changes
Poem
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 (
|
✅ Deploy Preview for preact-signals ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/react/src/lib/index.ts (2)
30-32: Document the reason for @ts-ignoreWhile the interface extension is valid, please add a comment explaining why the @ts-ignore directive is necessary. This will help future maintainers understand the type system constraints that necessitate this suppression.
- // @ts-ignore + // @ts-ignore - Required to prevent circular type reference between Signal and ReactElement
Line range hint
1-35: Consider documenting type system couplingThe extension of both Signal interfaces with ReactElement creates a tight coupling between Preact's signal system and React's type system. Consider:
- Adding a comment block at the top of the file documenting this architectural decision
- Creating a type-test file to ensure this compatibility is maintained in future updates
- Documenting any performance implications for type checking
This will help maintain the type safety guarantees as both libraries evolve.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.changeset/odd-fireants-rush.md(1 hunks)packages/react/src/lib/index.ts(1 hunks)
🧰 Additional context used
🪛 LanguageTool
.changeset/odd-fireants-rush.md
[style] ~5-~5: This phrase is redundant. Consider using “inside”.
Context: ... Fixed type mimatch for ReadonlySignals inside of JSX
(OUTSIDE_OF)
🔇 Additional comments (1)
packages/react/src/lib/index.ts (1)
33-35: LGTM! Verify JSX compatibility
The addition of ReactElement extension to ReadonlySignal maintains consistency with Signal and should resolve the type mismatch in JSX contexts.
Let's verify the JSX usage patterns in the codebase:
✅ Verification successful
Based on the extensive search results, I can now provide a definitive response about the JSX compatibility of the ReadonlySignal interface:
JSX compatibility is correctly implemented
The ReadonlySignal interface extending ReactElement is consistent with the codebase's JSX integration pattern. Key evidence:
- The Signal prototype is decorated with React element properties in
packages/react/src/lib/tracking.ts:
Object.defineProperties(Signal.prototype, {
$$typeof: { configurable: true, value: ReactElemType },
type: { configurable: true, value: SignalValue }
})- JSX rendering tests in
packages/react/test/browser/updates.test.tsxconfirm signals work as JSX elements:
it("should render Signals as SignalValue", async () => {
const sig = signal("test");
await render(<span>{sig}</span>);
// ...
})The implementation ensures signals can be used directly in JSX expressions while maintaining type safety through the ReactElement extension.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for JSX usage patterns with ReadonlySignal
# Look for places where ReadonlySignal is used in JSX context to ensure compatibility
# Search for ReadonlySignal usage in TSX/JSX files
rg -t tsx -t jsx "ReadonlySignal" -A 5
# Search for potential signal usage in JSX expressions
ast-grep --pattern '{$_: ReadonlySignal<$_>}'
Length of output: 114
Script:
#!/bin/bash
# Let's try a broader search approach without file type restrictions
# First, find all TypeScript/JavaScript files
fd "\.(ts|tsx|js|jsx)$" -x rg -l "ReadonlySignal"
# Then search for Signal usage patterns in JSX/TSX context
rg "Signal.*>" -A 3
# Look for type definitions and interfaces related to ReadonlySignal
rg "interface.*ReadonlySignal" -A 3
# Search for potential JSX element usage
rg "<.*Signal.*>" -A 3
Length of output: 103325
Summary by CodeRabbit
New Features
ReadonlySignalinterface to extendReactElement, enhancing compatibility with React's type system.Bug Fixes
ReadonlySignalsin JSX.