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

Server Side Rendering Parent Attributes Context #19685

Closed
epiqueras opened this issue Jan 15, 2020 · 26 comments · Fixed by #21467
Closed

Server Side Rendering Parent Attributes Context #19685

epiqueras opened this issue Jan 15, 2020 · 26 comments · Fixed by #21467
Assignees
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Status] In Progress Tracking issues with work in progress [Type] Performance Related to performance efforts
Milestone

Comments

@epiqueras
Copy link
Contributor

The Problem

A lot of patterns have surfaced where we need inner blocks to be aware of specific values configured on their parents.

For example, the Post block needs to set a post ID context value for a Post Title block child.

This introspection is hard to do because rendering happens depth-first, so child render-callbacks run before their parents run theirs. Blocks like the navigation block get around this by rendering their children themselves.

Imagine having a 3 level pyramid of blocks implementing this pattern:

                   
            1A     
           /--\    
         /-    -\  
       --        --
       2A        2B
     /-|-\         
   /-  |  -\       
 --    |    --     
 3A    3B   3C     

The callbacks would run in the following order:

3A
3B
3C
2A
+3A
+3B
+3C
2B
1A
+3A
+3B
+3C
+2A
+3A
+3B
+3C
+2B

The + signs represent the extra re-renders caused by this pattern. The approach converted this linear computation into something that scales with the square of the number of levels times the average number of children per node?

A Solution

I propose an alternative way for parents to provide data to their children during server-side rendering. It would require minimal changes to Core, and it fits in nicely with our heavy client-side usage of React Context.

Basically, we should extend render_block so that it's last parameter is a union of its parents' attributes:

render_block( $block['innerBlocks'][ $index++ ], $parent_attributes . $block['attrs']  )

This approach would allow blocks like Post Title to check for the nearest parent provided post ID easily, and the Navigation Link block to access the provided class name and style attributes so that they can render themselves correctly.

@epiqueras epiqueras self-assigned this Jan 15, 2020
@epiqueras epiqueras added [Type] Performance Related to performance efforts Framework Issues related to broader framework topics, especially as it relates to javascript [Feature] Full Site Editing labels Jan 15, 2020
@epiqueras epiqueras added this to the Future milestone Jan 15, 2020
@epiqueras
Copy link
Contributor Author

cc @youknowriad @mtias @aduth

@Soean Soean added [Package] Server Side Render /packages/server-side-render and removed [Package] Server Side Render /packages/server-side-render labels Jan 15, 2020
@epiqueras
Copy link
Contributor Author

We could also make things more explicit like React.

The parent block can register attributes to provide as context:

{
	"name": "core/post",
	"category": "layout",
	"attributes": {
		"postId": {
			"type": "number",
			"context": true
		}
	}
}

Then children block can explicitly consume them:

{
	"name": "core/post-title",
	"category": "layout",
	"context": {
		"postId": "core/post"
	}
}

@epiqueras
Copy link
Contributor Author

Then the signature would be:

render_block( $block['innerBlocks'][ $index++ ], $context  )

@chrisvanpatten
Copy link
Member

The idea of explicitly registered context feels way more intuitive than merging attributes. I really like that API.

@youknowriad
Copy link
Contributor

It raises one question here, if this is a declarative API (block registration) (like the above), it could work for both frontend and backend in a transparent way. (I haven't made my mind yet whether it's the best approach. It comes with the cost that it's not flexible sometimes, so what are the limits: what If I want to set a context value that is not an attribute?

@epiqueras
Copy link
Contributor Author

what If I want to set a context value that is not an attribute?

When would that ever be a need? We need them to be "attributes" so that they persist between parses and serializations.

@epiqueras
Copy link
Contributor Author

I found some time to flesh this idea (#19685 (comment)) out, and I am thrilled with the results. I tried it with the Post, Post Title, and Post Content blocks. I also added support for an array of block types for consuming context from:

{
	"name": "core/post-title",
	"category": "layout",
	"context": {
		"postId": [ "core/post", "some-plugin/fancy-post" ]
	}
}

The resolution will run left-to-right, only overriding previous values if set. Having this at the registration level is useful in Core for some complex blocks, but even more so for sharing parent and child blocks amongst Core and different plugins. The configuration can be filtered to both provide more/fewer attributes and to consume more/fewer attributes from more/fewer blocks.

  1. Lib: Implement server-side block context.
    This commit implements the block context API in the server. It was surprisingly brief.

  2. Block Editor: Implement client-side block context.
    This commit implements the block context API in the client. Note how we leverage React Context to create a provider per context attribute and have changes propagate correctly.

  3. Editor: Provide root Post block context.
    This commit adds providers for the Post block's context at the root of the editor with values sourced from the edited post. We need this so that we can still use things like Post Title blocks at the root level and have them read/update the root post.

  4. Core Data: Support passing a specific ID to entity hooks instead of reading from providers.
    This commit extends entity hooks by supporting specific IDs as parameters that will override any provided values from entity providers. We need this because the Post block will provide both post type and post ID using block context so there is no longer a need for an entity provider.

  5. Block Library: Implement Post blocks context server-side.
    This commit implements Post block server-side rendering using the new block context.

  6. Block Library: Implement Post blocks context client-side.
    This commit implements Post block client-side rendering using the new block context. Notice how easy it was to add support for multiple post types.

I think this a significant step forward with our APIs. The benefits of this over current approaches are evident:

  • More declarative, less verbose code.
  • Isomorphic API/logic that ties in well with the rest of the blocks APIs.
  • Easy to provide multiple entity parameters to child blocks, E.g., post type, and ID.
    • Doing this with entity providers would require us to have a kind context that sets the type and id for you. It would work, and we probably want to do it anyway if we keep the entity provider abstraction, but it would still not scale well to other use cases.
  • Easy for multiple Core and plugin blocks to reuse blocks that provide or consume values.
    • Doing this with regular providers would be impossible without completely hijacking edit implementations.

I think the last two are essential enough for me to believe this is the optimal direction.

I am not sure how valuable entity providers will be if we go forward with this. Perhaps, they will be used outside of blocks, for other UI? If that's the case, we should still look at adding that kind context I mentioned, and maybe even a "root" one that also provides the kind for you. Otherwise, we could plan to deprecate them.

@mcsf
Copy link
Contributor

mcsf commented Jan 20, 2020

Thanks for the deep dive.

  • Easy to provide multiple entity parameters to child blocks, E.g., post type, and ID.
    • Doing this with entity providers would require us to have a kind context that sets the type and id for you. It would work, and we probably want to do it anyway if we keep the entity provider abstraction, but it would still not scale well to other use cases.

Could you elaborate? In addition:

if we keep the entity provider abstraction

Is it on the table to not keep it?

{
	"context": {
		"postId": [ "core/post", "some-plugin/fancy-post" ]
	}
}

I'm curious about the use cases for multiple sources for resolution. For two reasons:

  1. It should be possible to any third-party block such as fancy-post to internally consume and use as provider core/post.
  2. Are there compelling enough use cases where there should be multiple sources? I'd lean towards something more restrictive.

I am not sure how valuable entity providers will be if we go forward with this. Perhaps, they will be used outside of blocks, for other UI? If that's the case, we should still look at adding that kind context I mentioned, and maybe even a "root" one that also provides the kind for you. Otherwise, we could plan to deprecate them.

This concerns me a bit. Aren't the two things reconcilable? Otherwise it sounds like we either got the entity-provider API all wrong, or we're now taking a direction that is too focused.

@epiqueras
Copy link
Contributor Author

Could you elaborate? In addition:

With an EntityProvider it would look something like this:

function Parent( { attributes } ) {
	return (
		<EntityProvider
			kind="postType"
			type={ attributes.postType }
			id={ attributes.postId }
		>
			<InnerBlocks />
		</EntityProvider>
	);
}
function Child() {
	const prop = useEntityProp( 'postType', null, 'prop' );
	return prop;
}

It's not terrible, but if we need block context in the server, it'd be wasteful not also to use it in the client:

function Parent() {
	return <InnerBlocks />;
}
function Child( { context: { postType, postId } } ) {
	const prop = useEntityProp( 'postType', postType, 'prop', postId );
	return prop;
}

Furthermore, what if we rely on some attribute unrelated to entities, would we introduce a different provider?

Is it on the table to not keep it?

I don't think it will have any maintenance overhead, and it could be useful in non-block UIs, so I don't think we should deprecate it. But it's worth thinking about it.

It should be possible to any third-party block such as fancy-post to internally consume and use as provider core/post.
Are there compelling enough use cases where there should be multiple sources? I'd lean towards something more restrictive.

That is possible with this approach. I think you mixed things up here a bit. fancy-post is another post provider just like core/post.

Here is an excellent practical example:

Newspack implements a Homepage Article block, which functions like the Core Post block but has a lot more features specific to their clients. They'd like to reuse some of the Post field blocks like Post Title without having to clone them and pollute the inserter. They could filter its registration and change core/post for homepage-article, but that would stop it from working with the Core Post block. Allowing this setting to be an array lets them have it work with both.

This concerns me a bit. Aren't the two things reconcilable? Otherwise, it sounds like we either got the entity-provider API all wrong, or we're now taking a direction that is too focused.

The entity provider API is excellent and could continue to be valuable for building entity based UIs other than blocks. The issue with it in the context of block authoring is that there's no easy way to map it to the server rendering process. In the process of trying to find a solution for this, I realized that a more general block context could work for any attribute, not just entity API parameters and that the API can work for both server and client.

@epiqueras
Copy link
Contributor Author

It should be possible to any third-party block such as fancy-post to internally consume and use as provider core/post.

That is possible with this approach. I think you mixed things up here a bit. fancy-post is another post provider just like core/post.

Oh, I see what you mean now. The Newspack example also explains this. Forcing them to add a Core Post block inside their Homepage Article block would clutter the UI so we would probably need some concept of invisible blocks, and what if they want to position InnerBlocks differently? It just makes certain extensions very awkward, if not impossible.

@mcsf
Copy link
Contributor

mcsf commented Jan 20, 2020

Oh, I see what you mean now.

Yes, this is it. :)

Forcing them to add a Core Post block inside their Homepage Article block would clutter

Definitely, that wouldn't be ideal. But it still feels like allowing several block types to provide context to be the wrong workaround. What if Homepage Article could just borrow something from Core Post to achieve the same result semantically? Off the top of my head, <Post.Context>.

This may also encourage third-party blocks to follow the conventions and semantics of their closest core equivalents, and perhaps act as expansions thereof — rather than (more often than not incompatible) substitutes, which is easier to get away with if you can just add a new block type to the child block's context requirements.

@epiqueras
Copy link
Contributor Author

Interesting. It would have to be something like:

{
	"name": "core/post-content",
	"category": "layout",
	"contextName": "core/post"
}

So that it also works for server-side rendering.

I don't see how this will encourage the following of conventions or semantics. They can just as easily overwrite the context and go down the wrong path. But, a benefit I do see with this is that it's simpler than filtering every post field block.

There a quite a few limitations to this, though. It removes the possibility of them building blocks that only consume from homepage-article and do nothing with core/post. We could make these blocks using contextName provide for both their block name and the used one, but that's super confusing.

What if two post providing blocks share some consuming blocks and also have some that they don't share. You want to introduce a third post providing block that uses all of the consuming blocks. contextName would need to support an array.

There are also possibilities for breakage. If there are already two post providing blocks sharing consuming blocks and a plugin wants to introduce an extra consuming block that only works with 1, there would be no way of doing this without completely rewriting all the other block configurations.

I guess all of these are symptoms of the fact that if you allow blocks to share a providing context, you create dependencies on all current and future consuming blocks. My proposal doesn't have this issue because each block gets its context, and adding providers to consuming blocks doesn't implicitly create dependencies from future providing blocks and doesn't affect other consuming blocks.

@mcsf
Copy link
Contributor

mcsf commented Jan 22, 2020

I don't see how this will encourage the following of conventions or semantics. They can just as easily overwrite the context and go down the wrong path. But, a benefit I do see with this is that it's simpler than filtering every post field block.

If you are to expand on an established core-defined shape, then breaking that convention is something that can be called out as wrong. In contrast, if you're free to build an entirely different post-provider, you have all that freedom and flexibility, at the cost of framework-level commonalities. I don't know how significant its impact will be in practice, but the difference exists. ¯_(ツ)_/¯

There a quite a few limitations to this, though. It removes the possibility of them building blocks that only consume from homepage-article and do nothing with core/post.

What if it didn't? Initially I was going to propose an inheritance-type model in that a homepage-article is a core/post. However, we're likely all too familiar with the pitfalls of ill-designed inheritance. The next thing that one might reach for, then, is a traits model. So what if:

  • core/post acts as a post provider.
  • a child like core/post-content declares a need for context via something like contextName: 'core/post'
  • any third (or first, for that matter) party like homepage-article wishing to implement a different post-providing block can nevertheless declare that explicitly: providesContext: 'core/post'.
  • meanwhile, a third-party child can either request contextName: 'core/post' or contextName: 'homepage-article, depending on specificity.
  • one question is whether it should be left implicit that homepage-article can also provide its own context, or whether providesContext should be an array ([ 'core/post', 'homepage-article' ]).

That way, the onus isn't on the child to state all possible providers of post context. Instead, providers need to be clear about their data. They aren't forced to conform to core patterns and can diverge, but they are naturally encouraged to follow core patterns if they want to be adopted by users who want the ability to mix together parents and children from different block libraries (core and multiple third parties).

What if two post providing blocks share some consuming blocks and also have some that they don't share. You want to introduce a third post providing block that uses all of the consuming blocks. contextName would need to support an array.

Yeah, this seems very related to what I just described. :)

There are also possibilities for breakage.

For sure. We need to think this through.

I guess all of these are symptoms of the fact that if you allow blocks to share a providing context, you create dependencies on all current and future consuming blocks. My proposal doesn't have this issue because each block gets its context, and adding providers to consuming blocks doesn't implicitly create dependencies from future providing blocks and doesn't affect other consuming blocks.

👍

@aduth
Copy link
Member

aduth commented Mar 19, 2020

I'm coming to this a bit late. That said, the work here and in #19572 is looking good. Both the implementation and the use-cases ring reminiscent of React's context feature, which I'm sure is not unintentional.

I have a few initial observations:

  1. Declaring context needs from a child block.

I'm interested in this conversation about how a child is expected to declare their context needs. My first impression was that we don't need to be so strict about how and from where this context is assumed to come. If a title block needs a post ID, why can't it just declare as such:

{
	"name": "core/post-title",
	"context": [ "postId" ]
}

Aside: I expect we'd need or want to support multiple context values being consumed in a block, so being able to express it as an array (if even optionally) I feel would be wise.

The way I picture it is that it acts the same as React context, in that it will use the value of the closest ancestor block which provides an attribute context by that name. There could be multiple ancestors, or just one, or maybe even zero. The closest "wins".

To the question of: From where specifically is postId defined? I'm not really sure it matters. For the purpose of this post title block, we probably expect it to be a post block, sure. Does it need to be? I don't think so. Could it be a third-party block? Why not?

I guess there could be a potential concern for conflict, moreso in cases where a key could be ambiguous in how it's expected to be used. A context of type could mean many different things, and naively taking the closest context provider's type value may yield strange results. Based on my reading of the conversation here, it may be for this reason that the conventions around namespacing the keys is being proposed? If so, is it fair to say that core/post is not a reference to a block by the name of core/post, but rather something more of "the post value", scoped to the "core" scope? And it just so happens that this is the same as the name of the post block?

  1. The relation between context and attributes.

Considering again the similarities between this and React's context: In React, context and props are completely separate, and there is no overlap. In the current proposed implementation here, there is some overlap:

  • From the child block, the context it consumes is separate from its own attributes.
  • From the ancestor block, the context it provides is picked from its attributes.

As noted previously, there's a strong likelihood that the context that an ancestor provides will correspond directly with one of its own attributes.

I raise this not because I necessarily see it as a problem, but moreso just to highlight the inconsistency, because I could foresee this becoming a point of confusion.

There's also a question of: Is there ever a case where an ancestor might want to provide some context which isn't precisely the value of one of its own attributes?

Such as:

  • A derivation of its own attributes
  • Some combination of an attribute and its own context
  • Something totally ad hoc

A problem we'd likely encounter going down this route is that it's fairly straight-forward to compute a context from attributes consistently between server and client. To implement a derived or ad hoc value which can work both in the server and client is not nearly as clear, and would probably require the block developer to implement it separately in both places.

We could still define context as a top-level property of the block, mapping from context to attribute name:

{
	"name": "core/post",
	"attributes": {
		"postId": {
			"type": "number"
		}
	},
	"context": {
		"postId": "postId"
	}
}

There's pros and cons to this. It definitely feels redundant. But on the other hand, it elevates context to a separate and top-level property of a block type, reintroduces consistency of context being a separate concept from attributes, and leaves open some potential future of additional mapping sources.

  1. The impact of context on function signatures of block rendering.

With this, I am mostly talking about the server-side render_callback, and specifically this change:

d968c0c#diff-6ff32417da0658502e7caa1a1abbeae6R203

... where the render_callback signature becomes:

function render_callback( $attributes, $block_content, $block ) {}

It's the same as what was proposed with Trac#48104, and in many ways similar to the problems discussed in #19401.

The problem then and now is that there's high redundancy here, in that $attributes and $block_content are respectively the same as values $block['attributes'] and $block['inner_content'] of the third $block argument.

The root of the problem is that we should have always just given $block in the first place, which would have scaled to accommodate all of the properties which have been added over time.

Our options as they exist today are:

  • Continue to cherry-pick individual properties to include as individual arguments of render_callback (add something like $block_context as an argument to the function).
  • Compromise and give $block as an argument, even if redundant, making the best of the situation as the middle-ground between "we should have always given only $block" and the reality of the current situation.
  • Something more like with Block API: Deprecate block transform function in favor of convert #19401, where we either create a separate function or other mechanism of opting in to the signature of $block being the sole argument.

@youknowriad
Copy link
Contributor

"context": [ "postId" ]
"context": {
"postId": "postId"
}

You used the same key for the producer and the consumer, may need a suffix.

Our options as they exist today are:

There's another approach maybe less clean but common in WordPress, using a global variable you can access inside the render callback.


I'm also wondering whether the consumer need to declare anything in its setting or just pick from the context bucket as needed. Thinking more here I do like the declarative approach as it opens the door for smart behaviors in the inserter (show/hide blocks depending on context...)

Something more like with #19401, where we either create a separate function or other mechanism of opting in to the signature of $block being the sole argument.

I like this approach. 🤷‍♂

@aduth
Copy link
Member

aduth commented Mar 19, 2020

You used the same key for the producer and the consumer, may need a suffix.

I'm not sure I follow? They're the same because the consumer is consuming the key as provided from the provider.

Edit: I saw there was a typo in that I was using core/post-title in the second example of a provider, where I had intended core/post instead. I've since corrected the comment.

There's another approach maybe less clean but common in WordPress, using a global variable you can access inside the render callback.

😱

... but yes, technically it is an option 😄

There is prior art in global $post;, etc.

@youknowriad
Copy link
Contributor

I'm not sure I follow? They're the same because the consumer is consuming the key as provided from the provider.

What If a block wants to be a provider for something and a consumer for something else.

@aduth
Copy link
Member

aduth commented Mar 20, 2020

I'm not sure I follow? They're the same because the consumer is consuming the key as provided from the provider.

What If a block wants to be a provider for something and a consumer for something else.

Oh! You're right. I conflated the "context" key to mean both things in my examples. I haven't yet put too much thought into it, but I suppose we could address this by one of:

  1. Have separate keys to differentiate consuming from providing (providesContext: {}, context: [])
  2. Use a single context key, but structure the value such as to support expression of both providing and consuming (context: { provides: {}, consumes: [] })
  3. Or, back to the original proposal, incorporate context providing as part of the attributes schema, and thus context key is always reserved for consumers.

@aduth
Copy link
Member

aduth commented Mar 25, 2020

We could still define context as a top-level property of the block, mapping from context to attribute name:
...
There's pros and cons to this. It definitely feels redundant. But on the other hand, it elevates context to a separate and top-level property of a block type, reintroduces consistency of context being a separate concept from attributes, and leaves open some potential future of additional mapping sources.

On the point of redundancy, I think it could be simplified further if (at least for an initial implementation) we assume a mapping to attributes:

{
	"name": "core/post",
	"attributes": {
		"postId": {
			"type": "number"
		}
	},
	"providesContext": [ "postId" ]
}

This still supports expansion to support additional sources. For example, overloading such that the following expressions could potentially all be equivalent:

Implicitly from attributes, retaining attribute naming:

{ "providesContext": [ "postId" ] }

Implicitly from attributes, customizing context naming:

{ "providesContext": { "postId": "postId" } }

Explicitly from attributes, optionally customizing context naming:

{ "providesContext": { "postId": { "source": "attribute", "name" "postId" } } }

Considering again the need or feasibility of other sources of context (#19685 (comment)), I don't know that strictly-speaking these values would need to be serialized, at least in how they're evaluated server-side. I do think it may (needlessly?) complicate how this would be implemented universally, where if we can assume in both the server and client to source from a known set of values (attributes), the framework can easily handle how those values are sourced. To some extent, there are parallels between this and comment-based attributes storage vs. attribute sources (which are still known by the framework how to resolve... well, at least client-side).

I do think it would be important to find at least one compelling use-case before investing too much in this idea that we'd need additional sources of context values.

But even if considered on its own merits as an alternative to embedding a "context": true boolean in the attributes schema, I observe:

  • It's not more to write in this form than a "context": true in each attribute (often less, in fact)
  • It colocates values provided as context, for quick scanning
  • It creates a symmetry between an ancestor's providesContext property and a descendent's context
  • It reinforces the distinction of attributes and context as separate (though still optimizing as assumed that a context value from a provider is derived from one of its own attributes)

@aduth
Copy link
Member

aduth commented Mar 25, 2020

It's arguable whether the descendent should need to declare its own context needs. The need for it seems to come from technical limitations of working with React to be able to subscribe to changes in that context. Server-side, at least as considered to be implemented in #19572, it's merely used to know how to pick values from a shared set of context values. At a high level, I don't know that I would have any strong reservations against simply making that shared set available in its entirety.

But those React limitations being what they are, at least if we assume that the context values can change over the lifetime of the editor, I don't know that we have any option but to require it.

And granted, it may bring some benefit to have the option to inspect a block type to understand what it is expecting to be provided as context.

@aduth
Copy link
Member

aduth commented Mar 25, 2020

Considering #19685 (comment) :

Then the signature would be:

render_block( $block['innerBlocks'][ $index++ ], $context  )

From the perspective of the public-facing render_block function, I am wondering (doubting) whether we want this to be part of the function signature. In ad hoc usage of this function, I don't know that non-internal usage should be able to assign arbitrary values to be used for context, rather than expect these context values to only be assigned by virtue of the presence of some block which is defined as to provide that context.

Also, considering the scenario we're running in to with render_callback as described in #19685 (comment) (a continuously expanding set of arguments), I think we'd want to be proactively very intentional about arguments we add to this function.

I see a couple options here:

  • We could make this argument "internal". There's plenty of prior art of this with functions themselves (e.g. _wp_menu_output), but only sparingly few with arguments, and they're not as clearly documented as being private (e.g. esc_url's $_context).
  • We could use a global, optionally in combination with making this available for render_callback. It may be simple enough to emulate the equivalent of a recursively aggregating argument by "resetting" the global back to its parent's initial value once recursion has finished.

Pseudo-code:

function render_block( $block ) {
    global $block_context;
    $block_context_before = $block_context;

    $block_type = WP_Block_Type_Registry::get_instance()->get_registered( $block['blockName'] );
    foreach ( $block_type->providesContext as $attribute_name ) {
        $block_context[ $attribute_name ] = $block['attrs'][ $attribute_name ];
    }

    $block_content = '';
    foreach ( $block['innerContent'] as $chunk ) {
        $block_content .= render_block( $block['innerBlocks'][ $index++ ] );
    }

    $block_context = $block_context_before;
    
    return $block_content;
}

@gziolo
Copy link
Member

gziolo commented Mar 26, 2020

I think the recent bug fix for @wordpress/element serializer handling of multiple distinct contexts is a good use case to take into account here as well:
#21156

The biggest limitation of the proposed approach is that we use the name of the attribute as the name of the context, so when you have a few levels deep nesting, if you want to expose postId from 2 different blocks at different levels to use both of them in the most nested block you wouldn't be able to do so because of overriding. You could always mitigate it by aliasing but you would have to plan it ahead. I don't know if it something to worry about but I thought that I will share this use case for consideration.

@aduth
Copy link
Member

aduth commented Mar 26, 2020

@gziolo I think there are cases where we'd want to make it easy to intentionally override the value based on a name, but to your general point, I agree there is some potential for unintended conflict.

From my earlier comment:

I guess there could be a potential concern for conflict, moreso in cases where a key could be ambiguous in how it's expected to be used. A context of type could mean many different things, and naively taking the closest context provider's type value may yield strange results.

Is this the problem you had in mind? Do you have a use-case you can imagine where a deeply nested block would need to be aware of the post ID not of its parent, but of its grandparent? Is it something where you think we'd want a system of namespacing, or only that we'd need to be careful and intentional in the context names we choose?

@gziolo
Copy link
Member

gziolo commented Mar 26, 2020

Is this the problem you had in mind? Do you have a use-case you can imagine where a deeply nested block would need to be aware of the post ID not of its parent, but of its grandparent? Is it something where you think we'd want a system of namespacing, or only that we'd need to be careful and intentional in the context names we choose?

Yes, it's very similar. I guess @youknowriad or @mcsf could know best whether it's a valid concern. I don't quite know how it all exactly works at the moment, but I would prefer we confirm if it for template parts and post content upfront. Just thinking only about that, I can see that the page has type and id assigned, then template part has internally another type and id, inside you might want to use let's say block that would read type and id from the parent, now the question is if it can read the type and id for the page or it's overridden by the template part? It might be a non-issue and I don't understand it all fully. Someone could clarify this cascade and relations :)

@skorasaurus
Copy link
Member

skorasaurus commented Mar 26, 2020

@gziolo I think there are cases where we'd want to make it easy to intentionally override the value based on a name, but to your general point, I agree there is some potential for unintended conflict.

From my earlier comment:

I guess there could be a potential concern for conflict, moreso in cases where a key could be ambiguous in how it's expected to be used. A context of type could mean many different things, and naively taking the closest context provider's type value may yield strange results.

Is this the problem you had in mind? Do you have a use-case you can imagine where a deeply nested block would need to be aware of the post ID not of its parent, but of its grandparent? Is it something where you think we'd want a system of namespacing, or only that we'd need to be careful and intentional in the context names we choose?

I'm not sure yet if I'm understanding your terminology exactly (I'm still learning Gutenberg's in and outs) but I have different headings for pages depending on what their grandparent is (e.g. display 'democracy' header and its navigation menu on a page slug of site-name.org/democracy-2020/events/speaker-series... The speaker-series page is a grandchild descendant of democracy-2020); sorry if I totally misunderstand. (The pre-Gutenberg logic of this is in our theme at https://gitlab.com/cpl/tempera-nocopyrt/-/blob/a1afc094fa51f1a3f06e69cd650e016b02028f91/header.php#L60 )

@aduth
Copy link
Member

aduth commented Mar 31, 2020

@skorasaurus It does seem related. It's not clear based on how it's currently implemented if it might be solved differently in a fully block-based theme template. And if it would use the context idea explored here, whether it may be implemented using a different key to identify the post of the relevant ancestor. Otherwise, I think it would require that we allow access not only to the closest context provider which provides a value for that key, but instead the full chain of ancestors who do. I worry this may add some complexity, especially if there may be different solutions to this problem. It's a good use-case to consider, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript [Status] In Progress Tracking issues with work in progress [Type] Performance Related to performance efforts
Projects
None yet
8 participants