Try: Alternative attributes initialization

[aduth]

Closes #865
Related: #391

This pull request seeks to explore an alternative approach to initializing and serializing attributes. See #865 for more detail on motivation.

Specifically, the goals with these changes are:

• Clarify attributes initialization as a getter action
• Consolidate to single signature for attributes initialization
• Provide mechanism for default values
• Refactor encoded attributes as explicit opt-in
  • Many blocks will not need it, thereby being able to disregard second argument to getInitialAttributes
• Rename wp.blocks.query to wp.blocks.source in an effort to clarify their passiveness and use as extraction strategies
  • Also incidentally resolves shadowing issue with wp.blocks.query.query
• Limit capabilities of source matching to discourage assumption of DOM (example)
• Create distinction between "finding" and "getting" attributes and content, where the former is a blind query and the latter a tree path traversal
• Rename source matching functions to more verbose equivalents, for consistency with other methods and clarity of usage

Downsides being:

• Would we want to discourage block implementers having access to the parsed content node (first argument to getInitialAttributes)? Ideally they'd not be traversing it themselves, and we might like to have the option to change its structure without impacting backwards compatibility. My first approach passed content as the raw HTML string, but at this point we'd already have done a single HTML parse pass, and would presumably like to limit the number of parsing passes.
  • We have the option of treating source functions as curried, to be satisfied at the time that attributes initialization occurs, e.g. getNodeAttribute( 'img', 'src' )( node );, where block implementation only returns getNodeAttribute( 'img', 'src' )
• How would we implement this sort of matcher without a presumed DOM? Do we need more matchers, maybe like: nodeName: getNodeName( node, [ '*' ] )
• I'm not sure whether get / find distinction is one we want

Testing instructions:

This is not a complete implementation and will surely fail tests and usage in the editor. At this point it's more an exploration / proposal / example. Review code changes instead. Also note that the implementation of the image block's encoded align attribute is subject to change in future efforts to eliminate duplicated source of truth, but is not a focus of this pull request.

[ellatrix]

Clarify attributes initialization as a getter action

I like this much more.

[ellatrix]

Would we want to discourage block implementers having access to the parsed content node (first argument to getInitialAttributes)? Ideally they'd not be traversing it themselves, and we might like to have the option to change its structure without impacting backwards compatibility. My first approach passed content as the raw HTML string, but at this point we'd already have done a single HTML parse pass, and would presumably like to limit the number of parsing passes.

I agree with this. At some point we might not want to pass a node. I wonder if calling it something different would discourage block implementors inspecting the object and encourage only using our selectors.

[ellatrix]

How would we implement this sort of matcher without a presumed DOM? Do we need more matchers, maybe like: nodeName: getNodeName( node, [ '*' ] )

I'm not sure what you mean. Can't it be something like getType( internalObject ) or getTag( internalObject )?

[aduth]

How would we implement this sort of matcher without a presumed DOM? Do we need more matchers, maybe like: nodeName: getNodeName( node, [ '*' ] )

I'm not sure what you mean. Can't it be something like getType( internalObject ) or getTag( internalObject )?

It could. I struggled to think of other use-cases for this source matching, and kept it generic to equivalent extracting of nodeName. A lower-case tag name could be more expected for developers. Also, you raise an interesting point on what the "root" should be. In the current example of extracting image caption, we traverse [ 'figure', 'figcaption' ], but the figure is truly the "root" node. If we were to make this assumed, the second argument could be optional when getting details about the root node, i.e. getTagName( node ) instead of getTagName( node, [ '*' ] );, and caption: getEditableContent( node, [ 'figcaption' ] ).

[youknowriad]

You already know my thoughts here, this still feels an imperative approach, I still think declarative approach is better (consolidate our attributes property with all attribute's metadata)

[ellatrix]

We can also do something à la dangerouslySetInnerHTML.

{ _internal: HTMLElement }

[ellatrix]

@youknowriad Why does it feel imperative? In React you can also set the initial state in the constructor?

[youknowriad]

@iseulde Yes, and React is not fully declarative right. I'm just comparing the approach of this branch with having something like #865 (comment) (which is more "declarative" IMO)

[ellatrix]

Those were two separate questions, sorry. :)

[ellatrix]

I'm not sure I get what the difference is between having a wrapper function or having an object with each of the items having functions in terms of being declarative.

[youknowriad]

If you have a function like:

getInitialAttributes( content ) => object, the block implementer could write any imperative code inside the function to trigger any processing and side effect inside this function to return the initial attributes.

while having { source: attributeResolver, default: 'value' } and attributeResover being an object like { source: 'metadata' } or { source: 'content', parse: props( 'tagName' ) }. We can't have any imperative code in there.

I agree that we could consider getInitialAttributes as declarative if it's a pure function but the former takes this declarativeness a level deeper.

[ellatrix]

but the former takes this decorativeness a level deeper

Right, you can still do it, just not for the whole object at once, but for each attribute.

[aduth]

@youknowriad I see what you're getting at, but also don't entirely agree that it's necessarily imperative because it makes use of a function, since the intent of the function is in returning an object, not performing side-effects, in much the same way that React's render function is not imperative (despite its name).

I do agree that this could be clearer if it weren't a function at all. However, I think there's some value in marking this as an action that occurs in initializing a block. The attributes property doesn't serve much purpose except at this time. It could perhaps, if we choose to integrate inspector/advanced properties as part of the attributes of a block.

Another consideration contrasting the function and object is that the latter requires a fair bit more API awareness (developer overhead) to accommodate type coercion and defaulting. With a function, this becomes "just code" (object spread to default, parseInt et. al to coerce type).

[youknowriad]

@aduth I understand the points raised here and this "the latter requires a fair bit more API awareness (developer overhead) " is a good point.

But I'd like to ask, how do you see us implement the inspector config?

[ellatrix]

I find getInitialAttributes a bit easier get what is happening, and you don't have to stick with what the query functions return. E.g. I could have a level attribute for headings be an integer derived from what a query function returns, whereas with the object I am forced to use the tag.

[aduth]

But I'd like to ask, how do you see us implement the inspector config?

Related: #672

I'm not totally sure. It's good to bring up. I'd remarked previously at #714 (comment). My uncertainty there on how we plan to use the Inspector still stands; when we change something in the inspector, what does it affect? I don't think it's any different than what we have now: it sets an attribute, which is either serialized into the markup or encoded in the block string. This itself is in no way incompatible with getInitialAttributes as a getter.

Where it gets interesting is if we wanted to consolidate inspector logic into an attributes field to allow automated field generation. It's an interesting idea, though in my experience can be burdensome and inflexible (order, styling, custom fields). If we were to do something like this, it might be wise to subscribe to JSON schema (already used in core APIs) and perhaps an existing form builder utility.

To me, the obvious alternatives are:

• inspector as a separate descriptor object, a subset of attributes, formatted as JSON schema, or an array of schemas corresponding to the fields shown
• Inspector as a Fill to be rendered by edit, using common form components
• inspector as an optional render method akin edit and save, also using common form components

[youknowriad]

@aduth That's one of the reasons these discussions are so great. Reading your comment here made me change my mind :).

One of my main arguments for the attributes property was having all the attributes metadata in a unique place. but your propositions regarding the inspector make me think we might not need any other metadata at all. I especially like the flexibility of your 2nd and 3rd propositions.

[aduth]

Closing in favor of #1905