Element: Fix serializer handling of multiple distinct contexts #21156
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aduth
added
[Type] Bug
aduth
requested review from
ajitbohra,
jorgefilipecosta and
youknowriad
as code owners
|
Size Change: +6.51 kB (0%) Total Size: 863 kB
ℹ️ View Unchanged
|
6 tasks
gziolo
reviewed
Mar 26, 2020
| Provider: SecondProvider, | ||
| } = createContext(); | ||
|
|
||
| const result = renderElement( |
There was a problem hiding this comment.
I see that e2e tests fail for blocks that have nesting. I'm wondering if there should be another unit tests that replicate how InnerBlocks works, if I follow it properly it would be something like:
<FirstProvider value="First">
<FirstConsumer>
{ ( first ) => (
<SecondProvider value="Second">
<SecondConsumer>
{ ( second ) => `First: { first }, Second: ${ second }` }
</SecondConsumer>
</SecondProvider>
) }
</FirstConsumer>
</FirstProvider>Although, I don't see any reason why it would make any difference.
There was a problem hiding this comment.
I'm not really sure what's going on with the end-to-end tests, to be honest. I've not been able to reproduce those same failures in my local environment.
There was a problem hiding this comment.
I added a test case for this in 90dacf0 for good measure. It passes, as expected. Still struggling with the end-to-end tests in Travis.
There was a problem hiding this comment.
I've just realized that we use one shared context for InnerBlocks so the following might be closer to the use case that fails on Travis:
<Provider value="outer">
<Consumer>
{ ( outter ) => (
< Provider value="inner">
<Consumer>
{ ( inner ) => `Outer: { outer }, Inner: ${ inner }` }
</Consumer>
</Provider>
) }
</Consumer>
</Provider>I don't think it's much different from a similar existing test though. I can't think of any reason why it fails on Travis :(
aduth
force-pushed
the
fix/serialize-multi-context
branch
from
7704924 to
6313fad
Compare
Not officially valid, yet technically supported in React's implementation. See: https://github.com/facebook/react/blob/a16b34974508cd23ce0844ad09a0e37a879d5591/packages/react-dom/src/server/ReactPartialRenderer.js#L1158-L1160
Multiple references in React's codebase check whether this is assigned. See: https://github.com/facebook/react/blob/a16b34974508cd23ce0844ad09a0e37a879d5591/packages/react-dom/src/server/ReactPartialRenderer.js#L1157 It's not entirely clear the exact circumstances this may occur at this point in code. Ideally there would be a test case verifying failure without this guard.
ellatrix
reviewed
May 20, 2020
| @@ -36,6 +36,7 @@ import { | |||
| kebabCase, | |||
| isPlainObject, | |||
| } from 'lodash'; | |||
| import CoreJSMap from 'core-js-pure/features/map'; | |||
There was a problem hiding this comment.
It's meant to be temporary, an experiment to see if it would fix the previously-observed issue to use a very predictable implementation.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #20721 (comment)
Previously: #8189, #5897
This pull request seeks to resolve an issue with the implementation of the custom
@wordpress/elementserializer where the a context consumer is only able to consume the value of the closest context provider, even if it is not of the same context type. As can be observed in the included test case, the result was that two distinct context providers would not interoperate.Implementation Notes:
As described at #20721 (comment), the underlying problem is that when a provider renders its children, it replaces the current context recursion argument, which prevents multiple provider values from coexisting in the same element tree.
The proposed changes seek to resolve this by changing the context argument to a
Mapobject, where a provider extends the map with its value. At the time the consumer is rendered, it performs a lookup of the provider's value using the components internaltype._context.Providervalue.Testing Instructions:
Ensure unit tests pass: