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(web): broken at dropping #3125

Merged
merged 4 commits into from
Oct 8, 2024
Merged

fix(web): broken at dropping #3125

merged 4 commits into from
Oct 8, 2024

Conversation

MJRT
Copy link
Contributor

@MJRT MJRT commented Sep 25, 2024

Description

After upgraded RN from 0.74 to 0.75 ( follow expo changelog), web page is broken when I do a drag and drop.

console log: https://gist.github.com/MJRT/149969001b9dd853e6b7d0d413afcea7
expo-device-info: https://gist.github.com/MJRT/2b772c0879f0a867ffb6bcf4a18d8637

It return to normal after adding this check.

Test plan

I use pnpm patch in my project, and it's all ok.
Since this is a simple and non-invasive change, I didn't write additional test.

@m-bert
Copy link
Contributor

m-bert commented Sep 25, 2024

Hi @MJRT, thank you for this PR 😄

web page is broken when I do a drag and drop.

Do you mean Drag & drop example from our example app?

I've seen this error a few times but it happened more-less randomly and it would be great if you could provide some reproduction code - maybe we will find root cause instead of just adding a null check 😅

@MJRT
Copy link
Contributor Author

MJRT commented Sep 25, 2024

Drag & drop example from our example app?

I haven't tried this example yet, but this error can stable reproduce in my project after upgrade to RN 0.75 and never see it on RN 0.74.

I’ll extract some code to reproduce it in a while. But I’m quite busy these days, so it might take a few days

@m-bert
Copy link
Contributor

m-bert commented Sep 25, 2024

I’ll extract some code to reproduce it in a while. But I’m quite busy these days, so it might take a few days

It's okay 😅 If I come across this error I'll let you know.

@m-bert
Copy link
Contributor

m-bert commented Oct 3, 2024

Hi @MJRT! I think I've found out what was the problem in our case.

Under some circumstances we enter this if statement, which basically means that gestureHandler is not initialized.

The problem is, currently it is a silent fail and we would like to change it to Error instead. However I'm not sure what caused that crash in your case and it would be great to check that before doing so.

@MJRT
Copy link
Contributor Author

MJRT commented Oct 6, 2024

Hi @m-bert , thanks for your sync, I extract a little code to reproduce this issue: https://github.com/MJRT/gesture-handle-web-broken-demo

it may have relation with I use lucide icons, you can stable reproduce it when switch tabs.

And I think we best can address this situation, that bring consistent dev experience, instead of handle a exception alone on web

@m-bert
Copy link
Contributor

m-bert commented Oct 7, 2024

Okay, I've checked your repro and I know what happened. For some reason <svg> is not treated as HTMLElement (it isSVGElement instead), so there was a silent fail on the line that I've mentioned earlier.

Now, we would like to give you opportunity to contribute to our library, but we do want to solve it in a different way than you proposed. Would you mind if I edit this PR?

@MJRT
Copy link
Contributor Author

MJRT commented Oct 7, 2024

Sure, tell me what I can do to help. I’m eager to contribute and collaborate on this solution.

@m-bert m-bert self-requested a review October 8, 2024 11:40
@m-bert m-bert requested a review from j-piasecki October 8, 2024 11:44
@m-bert
Copy link
Contributor

m-bert commented Oct 8, 2024

Okay, I've introduced required changes. Turns out that there was more to it than meets the eye so I've decided to do dirty work by myself. Thank you once more for spotting this problem and submittin PR ❤️

@m-bert m-bert merged commit 671b66f into software-mansion:main Oct 8, 2024
3 checks passed
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.

3 participants