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.

Sign up for GitHub

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

Jump to bottom

fix(FileUpload): revert to not calling onChange on file remove #2521

Merged
merged 2 commits into from
Feb 5, 2025
Merged

fix(FileUpload): revert to not calling onChange on file remove #2521

merged 2 commits into from
Feb 5, 2025

Conversation

saurabhdaware
Copy link
Member

Description

Issue Context: https://razorpay.slack.com/archives/CMQ3RBHEU/p1738568779980839?thread_ts=1738140484.133379&cid=CMQ3RBHEU

Timeline:

In 11.34.1: We did not call onChange on removing of file. Only onRemove was called
In 11:36.2: We added dispatchEvent call which started calling onChange on onRemove
In 12.4.0: We released a fix thinking onChange call was expected behaviour and we just updated the state value for it

So for consumers upgrading from 11.34.1 -> 12.4.0, it becomes breaking change because onChange was not previously called

Why is onChange getting called on dispatchEvent?

Hmm this is wild. So React apparently treats dispatchEvent with input type="text" differently than it treats input type="file"

Reproduction: https://codesandbox.io/p/sandbox/friendly-ishizaka-yk7mm3

I've added more details in issue here - facebook/react#32300

Changes

  • For now, I am removing the dispatchEvent from FileUpload to unblock the dashboard team. We'll have to add it again though for analytics sdk
  • I am also removing flushSync changes because we added it to fix onChange arguments which is not needed now

New behaviour: We'll revert back to behaviour of 11.34.1 where onChange is not called on removing file

Additional Information

Component Checklist

  • Update Component Status Page
  • Perform Manual Testing in Other Browsers
  • Add KitchenSink Story
  • Add Interaction Tests (if applicable)
  • Add changeset

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Feb 4, 2025 •
edited
Loading

🦋 Changeset detected

Latest commit: 8fa9c0c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@razorpay/blade Patch

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

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 4, 2025 •
edited
Loading

✅ PR title follows Conventional Commits specification.

@codesandbox-ci CodeSandbox CI
Copy link

codesandbox-ci bot commented Feb 4, 2025 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 8fa9c0c:

Sandbox Source
razorpay/blade: basic Configuration

@rzpcibot
Copy link
Collaborator

rzpcibot commented Feb 4, 2025 •
edited
Loading

Bundle Size Report

Updated Components
Status Component Base Size (kb) Current Size (kb) Diff
⬇ FileUpload 17.866 17.765 -0.101 KB

Generated by 🚫 dangerJS against 8fa9c0c

anuraghazra
anuraghazra reviewed Feb 4, 2025
View reviewed changes
packages/blade/src/components/FileUpload/FileUpload.web.tsx
@@ -377,11 +372,8 @@ const _FileUpload: React.ForwardRefRenderFunction<BladeElementRef, FileUploadPro
size={size}
onRemove={() => {
const newFiles = selectedFiles.filter(({ id }) => id !== file.id);
flushSync(() => {
Copy link
Member

Choose a reason for hiding this comment

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

We are also removing flushSync?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup we added it to fix the onChange args right? now we are not calling onChange on remove

anuraghazra
anuraghazra previously approved these changes Feb 4, 2025
View reviewed changes
@saurabhdaware saurabhdaware added the P0 Critical label Feb 5, 2025
@saurabhdaware
Copy link
Member Author

I verified the changes. Seems to work fine. Lets merge

@saurabhdaware saurabhdaware marked this pull request as ready for review February 5, 2025 05:47
@saurabhdaware saurabhdaware added the Review - L2 Second level of review label Feb 5, 2025
@saurabhdaware saurabhdaware changed the title fix: revert to not calling onChange on file remove fix(FileUpload): revert to not calling onChange on file remove Feb 5, 2025
@saurabhdaware saurabhdaware merged commit 3be3f0f into master Feb 5, 2025
14 of 16 checks passed
@saurabhdaware saurabhdaware deleted the fix/fileupload-onchange branch February 5, 2025 07:22
@saurabhdaware saurabhdaware mentioned this pull request Feb 5, 2025
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anuraghazra anuraghazra anuraghazra approved these changes

@tewarig tewarig tewarig approved these changes

@chaitanyadeorukhkar chaitanyadeorukhkar Awaiting requested review from chaitanyadeorukhkar chaitanyadeorukhkar is a code owner

@kamleshchandnani kamleshchandnani Awaiting requested review from kamleshchandnani kamleshchandnani is a code owner

@snitin315 snitin315 Awaiting requested review from snitin315 snitin315 is a code owner

Assignees

@anuraghazra anuraghazra

@tewarig tewarig

Labels
P0 Critical Review - L2 Second level of review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@saurabhdaware @rzpcibot @anuraghazra @tewarig