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

Check for Suspense boundary in a root Container #16673

Merged
merged 1 commit into from
Sep 5, 2019

Conversation

sebmarkbage
Copy link
Collaborator

If we find a Container that might mean that we're on a node that is inside
a Suspense boundary that is directly inside the Container root.

Imagine the div is a Container and the span is a dehydrated instance:

<div>
  <!--$-->
  <span />
  <!--/$-->
</div>

There's no way to tests this yet since I'm not actually utilizing
the return value yet.

The solution is to just use the same path to check for a Suspense boundary
as if we find a parent instance.

If we find a Container that might mean that we're on a node that is inside
a Suspense boundary that is directly inside the Container root.

Imagine the div is a Container and the span is a dehydrated instance:

```
<div>
  <!--$-->
  <span />
  <!--/$-->
</div>
```

There's no way to tests this yet since I'm not actually utilizing
the return value yet.

The solution is to just use the same path to check for a Suspense boundary
as if we find a parent instance.
@sizebot
Copy link

sizebot commented Sep 5, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS against c946ef6

@sebmarkbage sebmarkbage merged commit e11bf42 into facebook:master Sep 5, 2019
sebmarkbage added a commit to sebmarkbage/react that referenced this pull request Sep 23, 2019
This is a follow up to facebook#16673 which didn't have a test because it wasn't
observable yet. This shows that it had a bug.
sebmarkbage added a commit to sebmarkbage/react that referenced this pull request Sep 23, 2019
This is a follow up to facebook#16673 which didn't have a test because it wasn't
observable yet. This shows that it had a bug.
sebmarkbage added a commit that referenced this pull request Sep 23, 2019
* Add Event Replaying Infra

* Wire up Roots and Suspense boundaries, to retry events, after they commit

* Replay discrete events in order in a separate scheduler callback

* Add continuous events

These events only replay their last target if the target is not yet
hydrated. That way we don't have to wait for a previously hovered
boundary before invoking the current target.

* Enable tests from before

These tests were written with replaying in mind and now we can properly
enable them.

* Unify replaying and dispatching

* Mark system flags as a replay and pass to legacy events

That way we can check if this is a replay and therefore needs a special
case. One such special case is "mouseover" where we check the
relatedTarget.

* Eagerly listen to all replayable events

To minimize breakages in a minor, I only do this for the new root APIs
since replaying only matters there anyway. Only if hydrating.

For Flare, I have to attach all active listeners since the current
system has one DOM listener for each. In a follow up I plan on optimizing
that by only attaching one if there's at least one active listener
which would allow us to start with only passive and then upgrade.

* Desperate attempt to save bytese

* Add test for mouseover replaying

We need to check if the "relatedTarget" is mounted due to how the old
event system dispatches from the "out" event.

* Fix for nested boundaries and suspense in root container

This is a follow up to #16673 which didn't have a test because it wasn't
observable yet. This shows that it had a bug.

* Rename RESPONDER_EVENT_SYSTEM to PLUGIN_EVENT_SYSTEM
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants