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

Defer again after initial defer has been completed #110

Closed
wants to merge 4 commits into from

Conversation

AdamRamberg
Copy link
Contributor

Before this PR the <DeferredMountProvider/> would never defer again after initial deferring has been completed. In practice this meant that if a new child with a <DeferredMount/> or with a <DeferredMountWithCallback/> was mounted added after initial deferring was completed, the useIsDeferring() hook would still return false.
This PR changes that behaviour and sets the defer state to true every time a new child with <DeferredMount/> or <DeferredMountWithCallback/> is mounted. An example of where this behaviour would be useful and expected is when you have routing inside a <DeferredMountProvider/>.

Copy link
Contributor

@marlonicus marlonicus left a comment

Choose a reason for hiding this comment

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

Nice catch! One comment on testing, but it all looks good to me otherwise.

it('defers again after initial defer has completed', () => {
const DeferConditionally: React.FC<{ mountDeferred: boolean }> = ({ mountDeferred }) => {
return (
<>
Copy link
Contributor

@marlonicus marlonicus Jan 17, 2023

Choose a reason for hiding this comment

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

I would maybe add a print out of the useIsDeffering() hook value so you can verify that it's being set correctly, and then later run some expects on it. wdyt?

Suggested change
<>
<>
<p>isDeferring: ${ useIsDeferring() }</p>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense to me! I fixed it in my latest commit. Tried to follow the convention already in place in other tests for checking this.

* Remove requestingToRun state, since it is no longer needed
@AdamRamberg
Copy link
Contributor Author

AdamRamberg commented Jan 17, 2023

@marlonicus As mentioned above, adjusted the test according to your feedback!

I also want to mention that I removed the requestingToRun state, since it is no longer needed (the 2nd effect will always first time anyways and after that the state works exactly like the isDeferring state).

@pirelenito
Copy link
Member

As we are deprecating @react-facet/deferred-mount with #59, I'm closing this PR.

@pirelenito pirelenito closed this Jan 18, 2024
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