Skip to content

fix: spring does not start #2372

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

Draft
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

dagatsoin
Copy link
Contributor

@dagatsoin dagatsoin commented May 15, 2025

Why

This PR fixes a bug where useSpring lead to unmount component (eg. blank page)

Related: #2371

What

The root cause was an instruction which removes the current update during the useIsomorphicLayoutEffect callback.

@kierancap I would like your opinion on this if you have a minute. I don't know exactly the impact of this.

Checklist

  • Demo added: check before/after the demo css-variables
  • Ready to be merged

Copy link

vercel bot commented May 15, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
react-spring ✅ Ready (Inspect) Visit Preview May 15, 2025 8:16pm

Copy link

changeset-bot bot commented May 15, 2025

⚠️ No Changeset found

Latest commit: d6ae0f1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@joshuaellis
Copy link
Member

Do we have a reproduction of this in a sandbox? if i look at the examples in the docs, they all appear to work 🤔

@dagatsoin
Copy link
Contributor Author

@joshuaellis yes,

  • for the version with a bug : css-variables demo
  • for the version without a bug : compile the demo of my branch and launch the css-variables demo

@kierancap
Copy link

I'll take a look at this properly in a few hours. That line in particular was taken from the stale PR we had for React 19 before mine. I assumed it was fixing a cleanup of the ref. Would be good to test properly without it.

Whatever the cause of this bug is - Seeing as the CI tests passed, we should try and enhance the coverage to try and replicate this via CI.

If you were able to recreate the bug consistently is there a particular case where it fails?

As Josh mentioned, the local tests and the deployed docs worked! Odd!

Thoughts?

@kierancap
Copy link

I've just had a proper look through - I think we can remove this safely - I don't think the cleanup is necessary as the next runthrough of updates will overwrite any previously parsed updates. So in terms of the logic for removing it, all good on my end.

One thing I would like to suggest though is to catch the crashing behaviour in a test (like i mentioned before). I guess this is dependant on what causes the crashing, i.e what error it is. What are you guys' thoughts on that? Worth doing?

@dagatsoin
Copy link
Contributor Author

dagatsoin commented May 16, 2025

@kierancap agree to add a test on this, but I would first document the internal a bit. It is not easy to grasp for everyone.
The test will be more relevant.

So, what this business code actually do?

PS: the current website is not up to date so it run with the v9
You have a link upper to see a repro case.

@kierancap
Copy link

Yeah agreed about documenting the internals. Based on my understanding the ref is just handling queuing updates. I think the cleanup line may have come from the thought of needing the clean up an update post process, but based on my glance yesterday it seems like thats unnecessary (I'm also guessing that the ref is used asynchronously in other parts too - hence the crashing)

As to the actual design of the code, I'm not totally sure - It's not well documented so I've just tried to peace together what I can based on the code itself.

The last updates to that system pre our PR was 5-6 years ago, so I doubt we can get any assistance on that either.

If @joshuaellis has any understanding about the spring update system, would be great to enlighten myself and we can write some docs outlining it internally

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