Skip to content

fix(shared): resolve circular dependencies #1150

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

Merged
merged 2 commits into from
Mar 18, 2021

Conversation

khmm12
Copy link
Contributor

@khmm12 khmm12 commented Sep 16, 2020

shared/helpers wants entire shared/globals inside isAnimatedString, shared/globals wants noop from shared/helper to assign exported variable willAdvance.

If shared/helpers is executed before shared/globals, willAdvance is initialized by given value. Otherwise willAdvance is initialized after noop inside shared/helpers and has no defined value.

The safest way is keep shared/helpers pure and move isAnimatedString to own module.

It fixes #1078

@aleclarson
Copy link
Contributor

aleclarson commented Sep 19, 2020

Do you have a git repo that reproduces the error in #1078, so I can clone it and test this fix?

@khmm12
Copy link
Contributor Author

khmm12 commented Sep 23, 2020

Hey @aleclarson 👋

Prepared a demo ;-)

https://github.com/khmm12/react-spring-circular-dependencies-issue

Reproducible on production bundle.

@khmm12 khmm12 force-pushed the fix/shared-circular-dependencies branch from 6e11e33 to d35fb1c Compare September 23, 2020 09:19
@khmm12
Copy link
Contributor Author

khmm12 commented Sep 23, 2020

Did rebase the branch

Copy link

@TheSisb TheSisb left a comment

Choose a reason for hiding this comment

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

I was going to PR the exact same change. Rubber stamp lgtm

Copy link

@louisgv louisgv left a comment

Choose a reason for hiding this comment

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

LGTM 👍 @khmm12 Can we get another quick rebase for this PR for @aleclarson to merge it?

`shared/helpers` wants entire `shared/globals` inside `isAnimatedString`, `shared/globals` wants `noop` from `shared/helper` to assign exported variable `willAdvance`.

If `shared/helpers` is executed before `shared/globals`, `willAdvance` is initialized by given value. Otherwise `willAdvance` is initialized after `noop` inside `shared/helpers` and has no defined value.

The safest way is keep `shared/helpers` pure and move `isAnimatedString` to own module.
@khmm12 khmm12 force-pushed the fix/shared-circular-dependencies branch from d35fb1c to bcf5e2f Compare October 13, 2020 19:24
@khmm12
Copy link
Contributor Author

khmm12 commented Oct 13, 2020

@louisgv Ready ;-)

@louisgv louisgv mentioned this pull request Oct 13, 2020
@joshuaellis

This comment has been minimized.

@joshuaellis joshuaellis marked this pull request as draft March 18, 2021 21:31
@joshuaellis joshuaellis added the v9 label Mar 18, 2021
@joshuaellis joshuaellis added this to the v9.0.0 milestone Mar 18, 2021
Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

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

Sorry its taken so long, but thank you for this fix.

@joshuaellis joshuaellis marked this pull request as ready for review March 18, 2021 22:16
@joshuaellis joshuaellis merged commit 5e9b094 into pmndrs:v9 Mar 18, 2021
@khmm12 khmm12 deleted the fix/shared-circular-dependencies branch March 25, 2021 08:04
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.

@react-spring/core or @react-spring/three production errors
6 participants