-
Notifications
You must be signed in to change notification settings - Fork 52
fix: resolve react-number-format compatibility by refining compositionEnd handling #147
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
base: master
Are you sure you want to change the base?
fix: resolve react-number-format compatibility by refining compositionEnd handling #147
Conversation
概述修改 变更
序列图sequenceDiagram
participant User
participant Input Component
participant formatValue Function
participant onChange Callback
rect rgb(240, 248, 255)
note over User,onChange Callback: 修复前(compositionEnd 早期返回)
User->>Input Component: 输入数字(通过 IME)
Input Component->>Input Component: compositionEnd 触发
Input Component->>Input Component: 早期返回,状态未更新
onChange Callback-->>User: onChange 未触发或延迟
end
rect rgb(245, 255, 245)
note over User,onChange Callback: 修复后(总是更新状态)
User->>Input Component: 输入数字(通过 IME)
Input Component->>Input Component: compositionEnd 触发
Input Component->>Input Component: 用 cutValue 更新内部状态
Input Component->>formatValue Function: 格式化当前值
formatValue Function-->>Input Component: 返回 formatValue
alt cutValue ≠ formatValue && inputRef 存在
Input Component->>onChange Callback: 调用 onChange
onChange Callback-->>User: 触发一次
else 否则
Input Component-->>User: 跳过 onChange
end
end
代码审查工作量估计🎯 2 (简单) | ⏱️ ~10 分钟 需额外关注的区域:
诗歌
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (1)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
Summary of ChangesHello @oraziospada17, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a regression where Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request aims to fix a regression with react-number-format by ensuring compositionEnd events are processed, while still preventing the double onChange firing for IME input. The approach is to always update the internal state but only trigger onChange if the value has changed.
However, the implementation of the value check (cutValue !== formatValue) has a potential flaw. Due to React's event batching, formatValue may be stale across both compositionend and change event handlers, leading to onChange being fired twice. This could affect both uncontrolled and controlled components, re-introducing the bug the change is meant to solve. I've left a critical comment with a more detailed explanation.
| if (inputRef.current && cutValue !== formatValue) { | ||
| resolveOnChange(inputRef.current, e, onChange, cutValue); | ||
| } |
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.
This condition may not reliably prevent double onChange events during IME composition. Both the compositionend event and the subsequent change event can trigger triggerChange before the component re-renders. In this scenario, formatValue will hold the stale value from the previous render for both calls, causing this condition to be true twice and onChange to be fired twice.
This could re-introduce the issue this PR is trying to solve for compositionend scenarios (#46587). A useRef to track the last reported value within the event cycle might be a more robust solution to prevent the double-firing.
Fixes ant-design/ant-design#46999
Problem
The fix for #46587 (preventing double onChange firing with Chinese input) introduced a regression where
compositionEndevents were completely skipped. This broke compatibility with libraries likereact-number-formatwhich rely on receiving these events to function correctly when wrapping the Input component.Solution
Instead of early returning when
source === 'compositionEnd', this PR modifies triggerChange to:setValue).onChangecallback if the value has actually changed.This approach solves both issues:
react-number-formatto work (solving #46999) because it now receives the necessary events.Summary by CodeRabbit
发布说明
✏️ Tip: You can customize this high-level summary in your review settings.