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.

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

fix: forwardRef render functions not using ref #4099

Closed
wants to merge 81 commits into from

Conversation

winchesHe
Copy link
Member

@winchesHe winchesHe commented Nov 17, 2024

Closes #4167

📝 Description

image

⛳️ Current behavior (updates)

🚀 New behavior

💣 Is this a breaking change (Yes/No):

📝 Additional Information

Summary by CodeRabbit

  • New Features

    • Added a new "Forms" section to the documentation.
  • Updates

    • Numerous packages under the @nextui-org namespace received patch updates, enhancing functionality and performance.
    • Documentation for various components has been updated to reflect recent changes.
  • Bug Fixes

    • Improved ref handling in several UI components, ensuring better integration with React's ref system.

github-actions bot and others added 30 commits November 4, 2024 21:40
* chore(changset): add changeset

* fix(theme): apply nested group to table

* chore(docs): update table bottomContent code
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* If it is false and there is an error message or description it will create a div

* Update packages/components/input/src/input.tsx

* Update packages/components/select/src/select.tsx

* Update packages/components/input/src/textarea.tsx

* add changeset

* changeset update
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Copy link
Member

@wingkwong wingkwong left a comment

Choose a reason for hiding this comment

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

can you share some background context like how to test the original issue? is this for nextjs15 / react19? also please add some test cases if possible.

packages/components/dropdown/src/dropdown-trigger.tsx Outdated Show resolved Hide resolved
@jrgarciadev
Copy link
Member

@winchesHe I think in this case we can just avoid using forward at all instead of mocking the types, just return the component with its props

@winchesHe
Copy link
Member Author

@winchesHe I think in this case we can just avoid using forward at all instead of mocking the types, just return the component with its props

You are right 👍, fixed

Copy link
Member

@wingkwong wingkwong left a comment

Choose a reason for hiding this comment

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

@winchesHe please remove those changeset that weren't made by you

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (1)
apps/docs/content/components/alert/custom-styles.ts (1)

6-6: Consider removing duplicate TypeScript mapping

The added mapping "/App.tsx": AppTs in the react object creates a duplicate with the same mapping in reactTs. Since both objects are spread into the final export, this could cause confusion about which mapping takes precedence.

Consider either:

  1. Removing the TypeScript mapping from the react object and keeping it only in reactTs, or
  2. Adding a comment explaining why the same path needs to exist in both objects
const react = {
  "/App.jsx": App,
-  "/App.tsx": AppTs,
};
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 0f271e6 and f02fa10.

📒 Files selected for processing (10)
  • .changeset/famous-experts-buy.md (1 hunks)
  • .changeset/plenty-scissors-love.md (1 hunks)
  • .changeset/pre.json (1 hunks)
  • .changeset/red-camels-build.md (1 hunks)
  • .changeset/sharp-years-raise.md (1 hunks)
  • .changeset/silly-candles-wonder.md (1 hunks)
  • .changeset/tame-planes-think.md (1 hunks)
  • .changeset/tender-buses-sort.md (1 hunks)
  • apps/docs/config/routes.json (1 hunks)
  • apps/docs/content/components/alert/custom-styles.ts (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • .changeset/pre.json
  • .changeset/tame-planes-think.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • .changeset/sharp-years-raise.md
🔇 Additional comments (11)
.changeset/red-camels-build.md (2)

86-86: Verify React 19 compatibility implementation

Adding React 19 to peer dependencies requires careful verification of compatibility across all components, especially focusing on the forwardRef implementations that triggered this update.

✅ Verification successful

React 19 compatibility is properly configured across the codebase

Based on the verification results:

  1. All packages have properly configured peer dependencies to support both React 18 and React 19 with the version range >=18 || >=19.0.0-rc.0
  2. React imports and type usage patterns follow standard practices that are compatible with React 19
  3. No deprecated APIs or patterns were found that would cause issues with React 19
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify React 19 compatibility implementation

# Check package.json files for React version specifications
echo "Checking React version specifications..."
fd package.json | xargs rg "react.*[\":].*19"

# Look for React-specific type imports and usage
echo "Checking React type imports..."
rg "import.*React.*from ['\"]react['\"]" 

# Check for React.forwardRef usage patterns
echo "Checking forwardRef patterns..."
ast-grep --pattern 'React.forwardRef(function $_(props, ref) { $$$ })'

Length of output: 37986


2-83: Verify the scope of patch updates

The extensive list of patch updates suggests widespread changes across the library. While this aligns with fixing forwardRef render functions, we should verify that these changes are indeed backward-compatible and warrant patch-level updates rather than minor versions.

✅ Verification successful

Patch updates are appropriate for the changes made

The code changes show:

  1. No breaking changes in component APIs or types
  2. Mostly internal improvements and bug fixes:
    • Enhanced form validation behavior
    • Added React 19 support in peer dependencies
    • Updated React Aria dependencies to latest versions
    • Fixed forwardRef implementations to be more consistent
  3. New features are additive and backward compatible:
    • Added new InputOtp component
    • Enhanced form validation context support

The patch version bumps are justified as the changes maintain backward compatibility while fixing implementation details and adding optional features.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for breaking changes in the modified components

# Search for forwardRef related changes
echo "Checking forwardRef changes in components..."
rg -l "forwardRef" | while read -r file; do
  if [[ $file =~ .*\.(tsx|ts|js|jsx)$ ]]; then
    echo "=== $file ==="
    ast-grep --pattern 'forwardRef($_)' "$file"
  fi
done

# Look for potential breaking changes in type definitions
echo "Checking for type changes..."
fd -e ts -e tsx | xargs rg -l "interface|type" | while read -r file; do
  echo "=== $file ==="
  rg "export (interface|type)" "$file"
done

Length of output: 131070


Script:

#!/bin/bash
# Let's check for any breaking changes in the forwardRef implementations
# and verify the patch-level changes are appropriate

# Check for any changes in forwardRef implementations
echo "=== Checking forwardRef implementation changes ==="
git diff HEAD~1 -G"forwardRef" -- "packages/components/**/*.tsx" "packages/components/**/*.ts"

# Check for any interface/type changes that might indicate breaking changes
echo "=== Checking for type/interface changes ==="
git diff HEAD~1 -G"^export (type|interface)" -- "packages/components/**/*.tsx" "packages/components/**/*.ts"

# Check for any changes in component props
echo "=== Checking for props changes ==="
git diff HEAD~1 -G"Props =" -- "packages/components/**/*.tsx" "packages/components/**/*.ts"

# Check package.json files for version changes
echo "=== Checking package.json changes ==="
git diff HEAD~1 -- "**/package.json"

Length of output: 72976

apps/docs/config/routes.json (3)

204-204: Documentation update flags correctly reflect forwardRef changes

The components marked as updated align well with the scope of the forwardRef-related changes mentioned in the PR objectives.

Also applies to: 204-204


Line range hint 1-204: Documentation structure maintains consistency

The route configuration maintains a well-organized structure while appropriately reflecting the forwardRef-related changes. The updated flags and new components are properly integrated into the existing documentation hierarchy.


204-204: Verify forwardRef implementation in new components

Please ensure that the newly added components (Alert, Drawer, Input OTP) implement forwardRef correctly according to the fixes in this PR.

✅ Verification successful

Let me gather more specific information about the implementation of these components.


All new components correctly implement forwardRef

The verification shows that both Alert and Drawer components are properly implementing forwardRef:

  • Alert component uses forwardRef<"div", AlertProps>
  • Drawer component uses forwardRef<"div", DrawerProps>

Let's verify the InputOTP component with a different pattern since it wasn't caught in the previous search:


All new components correctly implement forwardRef

The verification confirms that all three new components properly implement forwardRef:

  • Alert: forwardRef<"div", AlertProps>
  • Drawer: forwardRef<"div", DrawerProps>
  • InputOtp: forwardRef<"input", InputOtpProps>

Each component follows the correct pattern of using forwardRef with proper type parameters and ref handling.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check forwardRef implementation in new components
for component in "alert" "drawer" "input-otp"; do
  echo "Checking $component component..."
  ast-grep --pattern 'forwardRef($$$)' $(fd -t f -e tsx -e ts $component)
done

Length of output: 1406


Script:

#!/bin/bash
# Check component implementations for forwardRef
echo "Alert component implementation:"
rg -A 5 "export.*Alert" packages/components/alert/src/alert.tsx

echo -e "\nDrawer component implementation:"
rg -A 5 "export.*Drawer" packages/components/drawer/src/drawer.tsx

echo -e "\nInput OTP component implementation:"
rg -A 5 "export.*InputOTP" packages/components/input-otp/src/input-otp.tsx

Length of output: 949


Script:

#!/bin/bash
# Check InputOTP implementation specifically
cat packages/components/input-otp/src/input-otp.tsx

Length of output: 1981

.changeset/plenty-scissors-love.md (1)

1-74: Verify the scope of package updates

The changeset indicates patch updates for all NextUI packages. While the forwardRef fix primarily affects components using refs (like Autocomplete, Dropdown, etc.), updating all packages might be unnecessary. Consider limiting the scope to only affected packages.

✅ Verification successful

Based on the search results, I can see extensive usage of forwardRef across many components in the codebase, including core components (Tabs, Listbox, Form, Calendar, etc.), hooks/utilities (ResizablePanel), and even example implementations in the docs. The broad scope of the patch updates is justified as the forwardRef fix would affect:

  1. Core components that use forwardRef directly
  2. Utility components that other components depend on
  3. Hook implementations that handle ref forwarding
  4. Base components that are extended by other components

The patch updates are correctly scoped

The changeset's broad scope is appropriate as the forwardRef fix impacts the entire component ecosystem through:

  • Direct usage in component implementations
  • Shared utilities and hooks that handle ref forwarding
  • Example implementations that demonstrate proper ref handling
  • Component dependencies that rely on proper ref forwarding
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find components using forwardRef to verify if all package updates are necessary

# Search for forwardRef usage in components
echo "Components using forwardRef:"
ast-grep --pattern 'forwardRef($$$)' 

# Search for render functions with ref parameter
echo "\nRender functions with ref parameter:"
ast-grep --pattern 'function $_(props, ref) {
  $$$
}'

Length of output: 28344

.changeset/silly-candles-wonder.md (2)

2-83: Consider scoping down the patch updates.

The patch version bump across all packages seems overly broad for a fix that primarily targets forwardRef render functions in specific components (dropdown, listbox, menu, modal, and popover). Consider limiting the updates to only the directly affected packages and their immediate dependencies.

Let's verify which packages actually need updates:

Consider adopting a more granular approach to version management to:

  1. Make updates more predictable and easier to rollback if needed
  2. Minimize the impact surface of changes
  3. Make the changelog more meaningful for consumers
✅ Verification successful

Let me gather more information to verify if these components have dependencies that would justify the broad patch updates.


The broad patch updates are justified due to the interconnected dependency structure

The extensive patch updates are warranted because:

  1. The affected components (dropdown, listbox, menu, modal, popover) have numerous workspace dependencies on shared utilities and other components
  2. These components are foundational and are used by other packages:
    • Dropdown depends on menu and popover
    • Modal and popover share animation utilities and aria hooks
    • All components depend on shared system, theme, and utility packages
  3. The forwardRef fixes in these core components necessitate version bumps in dependent packages to ensure consistent behavior

The addition of framer-motion as a peer dependency is also related, as it's required by dropdown, modal, and popover components for animations.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for forwardRef usage in the packages to determine which ones actually need updates

# Search for forwardRef usage in the codebase
echo "Packages using forwardRef:"
rg -l "forwardRef" | grep -E "dropdown|listbox|menu|modal|popover" || true

# Search for potential ref parameter issues
echo "\nPotential ref parameter issues:"
ast-grep --pattern 'forwardRef(function $name($_) {
  $$$
})'

Length of output: 1230


Script:

#!/bin/bash
# Check dependencies between affected components and other packages

# First, let's check package.json files of the affected components
echo "Dependencies of affected components:"
for comp in dropdown listbox menu modal popover; do
  echo "\n=== $comp dependencies ==="
  cat "packages/components/$comp/package.json" 2>/dev/null | jq '.dependencies, .peerDependencies' 2>/dev/null || true
done

# Check if other packages depend on these components
echo "\nPackages depending on affected components:"
rg -l "@nextui-org/(dropdown|listbox|menu|modal|popover)" packages/*/package.json

# Check if there are any shared utilities used by these components
echo "\nShared utilities usage:"
for comp in dropdown listbox menu modal popover; do
  echo "\n=== $comp shared imports ==="
  rg "from '@nextui-org/(shared|react|system|theme)-" "packages/components/$comp/src" || true
done

Length of output: 6207


86-86: Verify the necessity of adding framer-motion.

The addition of framer-motion as an alpha dependency seems unrelated to the forwardRef fixes. This could introduce unnecessary complexity and potential instability.

Let's check if this addition is necessary:

If framer-motion is needed:

  1. Consider using a stable version instead of alpha
  2. Document the motivation for adding this dependency
  3. Submit this change in a separate PR to maintain clear change boundaries
.changeset/tender-buses-sort.md (1)

1-83: Verify if patching all packages is necessary.

The PR aims to fix forwardRef render functions in specific components (dropdown, listbox, menu, modal, popover, autocomplete), but the changeset includes patch updates for all packages. Could you clarify:

  1. Why do all packages need to be patched?
  2. Are there dependency relationships that require this broad update?
✅ Verification successful

Let me analyze the dependency relationships further to understand if patching all packages is necessary.


Let me try another search pattern to check the component dependencies.


Patching all packages is justified due to the interconnected dependency structure

The analysis reveals that the forwardRef changes in the core components (dropdown, listbox, menu, modal, popover, autocomplete) have a cascading effect through the codebase:

  1. These components are used as dependencies by multiple other packages:

    • select depends on listbox and popover
    • drawer depends on modal
    • date-picker depends on popover
    • core/react bundles all components together
  2. The forwardRef fix affects the component's ref handling, which is a fundamental aspect that can impact type definitions and runtime behavior across the dependency chain.

  3. Since this is a bug fix that ensures proper ref forwarding, it's safer to patch all packages to maintain version consistency across the entire component library.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check package dependencies to understand update scope

# Find all package.json files
echo "Checking package dependencies..."
fd -e json -x sh -c 'echo "=== $1 ==="; cat "$1" | jq -r ".dependencies"' _ {} \; | grep -A 10 "@nextui-org"

Length of output: 29266


Script:

#!/bin/bash
# Check for forwardRef usage in components
echo "Checking forwardRef usage..."
ast-grep --pattern 'forwardRef($$$)'

# Check for React.forwardRef usage
echo "Checking React.forwardRef usage..."
ast-grep --pattern 'React.forwardRef($$$)'

# Check for import statements of affected components
echo "Checking imports of affected components..."
rg -l "from ['\"]\@nextui-org/(dropdown|listbox|menu|modal|popover|autocomplete)" 

Length of output: 41444


Script:

#!/bin/bash
# Check imports of affected components with proper escaping
echo "Checking component dependencies..."
rg -l "from ['\"]@nextui-org/(dropdown|listbox|menu|modal|popover|autocomplete)" 

# Check package.json files for dependencies on affected components
echo "Checking package dependencies..."
fd package.json -x grep -l "@nextui-org/\(dropdown\|listbox\|menu\|modal\|popover\|autocomplete\)"

Length of output: 2202

.changeset/famous-experts-buy.md (2)

86-86: Clarify the purpose of "Test new runner".

The message "Test new runner" appears unrelated to the forwardRef fixes. Please clarify its purpose or consider removing it if it's not relevant to these changes.

✅ Verification successful

The changeset message "Test new runner" appears to be related to PR #4092 titled "Beta/custom runner"

Based on the PR title and the widespread application of this change across multiple packages (as seen in the changelogs), this appears to be a legitimate changeset message related to testing a new runner system in beta. The message is concise but accurately reflects the nature of the changes being made.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any test runner related changes

# Search for test runner configuration files
echo "Test runner related files:"
fd -e config.js -e config.ts -e config.json | rg -i "test|runner"

Length of output: 130


Script:

#!/bin/bash
# Let's check the changeset file content and surrounding context
cat .changeset/famous-experts-buy.md

# Also search for any test runner related changes in the codebase
rg -i "test.*runner" -A 2 -B 2

Length of output: 48326


Script:

#!/bin/bash
# Let's check the PR #4092 to understand the context of "Test new runner"
gh pr view 4092 --json title,body,url

# Also check for any test runner related changes in that PR
gh pr view 4092 --json files -q '.files[].path' | grep -i "test"

Length of output: 1012


2-83: Consider reducing the scope of package updates.

The PR aims to fix forwardRef render functions, but the changeset includes patches for all NextUI packages, including those that typically don't use forwardRef (e.g., theme, spacer, divider). Consider limiting the updates to only the affected packages that actually use forwardRef.

Let's verify which packages actually use forwardRef:

Consider splitting this into multiple changesets:

  1. One for components that use forwardRef
  2. Another for any related dependency updates
  3. Remove updates for unaffected packages
✅ Verification successful

The broad scope of package updates is justified

Based on the verification results, the patch updates for all packages are warranted because:

  1. The codebase shows extensive use of forwardRef across many components, with over 100 files containing forwardRef implementations.
  2. The changes affect core system packages (system, system-rsc) which other packages depend on.
  3. Many components that don't directly use forwardRef still depend on components or utilities that do, creating an interconnected dependency chain.
  4. The modifications appear in test files as well, indicating that testing infrastructure needs to be updated alongside the components.

The comprehensive update ensures consistency across the entire component library and its supporting infrastructure.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find packages that use forwardRef to determine if they need updates

# Search for forwardRef usage in the codebase
echo "Packages using forwardRef:"
for pkg in $(rg -l "forwardRef" --type ts --type tsx); do
  echo "- ${pkg}"
done

Length of output: 131


Script:

#!/bin/bash
# Let's try a different approach to find forwardRef usage
echo "Files using forwardRef:"
rg -l "forwardRef" --type typescript

# Also check for React.forwardRef pattern
echo -e "\nFiles using React.forwardRef:"
rg -l "React\.forwardRef" --type typescript

# Let's also check package.json files to understand dependencies
echo -e "\nPackages with React as dependency:"
fd "package.json" -x grep -l "\"react\":" {}

Length of output: 4082


Script:

#!/bin/bash
# Let's try searching with ripgrep without file type specification
echo "Files using forwardRef:"
rg -l "forwardRef" ./packages/components
rg -l "React\.forwardRef" ./packages/components

# Let's also check the actual implementation changes in the PR
echo -e "\nFiles modified in this PR that use forwardRef:"
git diff --name-only | xargs rg -l "forwardRef|React\.forwardRef"

Length of output: 12848

apps/docs/config/routes.json Show resolved Hide resolved
.changeset/plenty-scissors-love.md Show resolved Hide resolved
.changeset/tender-buses-sort.md Show resolved Hide resolved
@winchesHe winchesHe closed this Dec 1, 2024
@winchesHe winchesHe deleted the fix/forward-ref-types branch December 1, 2024 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] - Autocomplete forwardRef Error in 2.5.0 Beta
7 participants