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

Framework: Use a simple JS object to declare the attribute's source #2854

Merged
merged 13 commits into from
Nov 16, 2017

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Oct 2, 2017

Description

This PR can be seen as the first step towards #2751

Instead of using the hpq matchers, we use a declarative API to define the attributes source

//before
import { attr } from 'api';

attributes: {
  url: {
    type: 'string',
    source: attr( 'src' ),
  }
}

// after
attributes: {
  url: {
    type: 'string',
    source: 'attribute',
    attribute: 'src',
  }
}

Caveats

A bit verbose compared to the previous approach, noticeable when nesting

// Gallery

images: {
  type: 'array',
  source: 'query',
  selector: 'div.wp-block-gallery figure.blocks-gallery-image img',
  query: {
    source: 'object',
    object: {
      url: {
        type: 'attribute',
        attribute: 'src',
      },
      alt: {
        type: 'attribute',
        attribute: 'alt',
      },
      id: {
        type: 'attribute',
        attribute: 'data-id',
      },
    },
  },
},

Pros

  • Permits server-side attribute declaration (which opens the way to server-side attribute parsing if necessary)
  • No hpq direct dependency, no need to wrap the matchers

Checklist:

  • Refactor the parser
  • Refactor the serializer
  • Refactor the blocks
  • Fix the unit tests and adds new tests
  • Update the documentation

@youknowriad youknowriad added Framework Issues related to broader framework topics, especially as it relates to javascript [Status] In Progress Tracking issues with work in progress labels Oct 2, 2017
@youknowriad youknowriad self-assigned this Oct 2, 2017
@youknowriad youknowriad requested a review from aduth October 2, 2017 12:24
@westonruter
Copy link
Member

If verbosity is a concern then in JS there could be shorthand “macros” like:

const attr = ( key ) => ( {
    type: 'attribute',
    attribute: key
} );

In that way you could still do:

attributes: {
  url: {
    type: 'string',
    source: attr( 'src' ),
  }
}

If you wanted to.

@aduth
Copy link
Member

aduth commented Oct 2, 2017

Talking with @mtias about this, it was mentioned maybe we don't need to nest source, and rather create the source-relevant properties on the same top-level, i.e.

// Before:
attributes: {
  url: {
    type: 'string',
    source: {
      type: 'attribute',
      attribute: 'src',
    }
  }
}

// After:
attributes: {
  url: {
    type: 'string',
    source: 'attribute',
    attribute: 'src',
  }
}

@youknowriad
Copy link
Contributor Author

@aduth How would you declare query( 'div', query( 'p', attr( 'className' ) ) )?

Nesting is needed IMO, and I personally prefer an explicit source property to declare the way we parse an attribute.

@ellatrix
Copy link
Member

ellatrix commented Oct 4, 2017

source: 'attribute',
attribute: 'src',

😵

@aduth
Copy link
Member

aduth commented Oct 4, 2017

I may be playing devil's advocate here, since I'd originally been a proponent of the nested source. But I'm trying to be sensitive to the impact of nesting, which I think can be harmful to general readability of the block definition and, from the perspective of the implementer, how obvious it is to structure. JSON schema is easy to pick on here, because it's... excessive ([1], [2]). While the requirements forced us to rethink the structure, I long for the simplicity we once had for defining attributes:

attributes: {
url: attr( 'img', 'src' ),
alt: attr( 'img', 'alt' ),
caption: html( 'figcaption' )
},

(Compare to current)

Of course, where we balance is on consistency and accommodating the varied needs of defining the source, where a nested object could have some merit.

@aduth How would you declare query( 'div', query( 'p', attr( 'className' ) ) )?

If we consider selector and source as the respective arguments to query:

{
	type: 'query',
	selector: 'div',
	source: {
		type: 'query',
		selector: 'p',
		source: {
			type: 'attribute',
			attribute: 'class',
		},
	},
}

Also worth considering optimizing for the more common scenario: query is the edge case, and has shown to have few use-cases thus far. Could even call into question its overall value and whether it's worth keeping around.

source: 'attribute',
attribute: 'src',

😵

You are going to have to elaborate here 😃 I assume you're not a fan of the redundancy? type may not be strictly necessary if we could infer the source type by the presence of other properties. But there is some value in being able to quickly determine (both visually for the reader, and programmatically) the type.

What would you see as the alternative here? type: 'attribute', name: 'src' ? Would it be as obvious the connection between the name property and the type of the source?

@ellatrix
Copy link
Member

ellatrix commented Oct 4, 2017

You are going to have to elaborate here 😃

I have no critique, I just thought it was funny. :)

@youknowriad
Copy link
Contributor Author

@aduth so for the first level, the source type is stored in "source", and the nested levels it's in "type". I don't like this because it's not consistent.

Overall what you propose is just considering the first level as a superset of "source".

@aduth
Copy link
Member

aduth commented Oct 9, 2017

The intent would be to optimize for the common use-case, which is not query. In retrospect, I can grant that it's not great for source to be either an object or a string. To a previous point, maybe the string version of source is redundant if it can be inferred from other properties. Or maybe we need to take a harder look at the use cases for query and whether they can be solved other ways (nested blocks?) so that it can be eliminated.

@aduth
Copy link
Member

aduth commented Oct 13, 2017

Actually, the non-nested query example can be written without introducing inconsistency to how source is used:

{
	type: 'array',
	source: 'query',
	selector: 'div',
	query: {
		source: 'query',
		selector: 'p',
		query: {
			source: 'attribute',
			attribute: 'class',
		},
	},
}

@youknowriad
Copy link
Contributor Author

@aduth Oh! nice proposal, I'll try to update accordingly.

@aduth
Copy link
Member

aduth commented Oct 13, 2017

Just now realizing I had type and source mixed up in both of my examples; didn't mean for there to be nearly as much inconsistency as there appears to be.

@youknowriad youknowriad force-pushed the try/declarative-attributese branch from 40d3d40 to f9c9356 Compare October 13, 2017 13:33
@youknowriad
Copy link
Contributor Author

Updated per suggestion

source: 'query',
selector: 'div.wp-block-gallery figure.blocks-gallery-image img',
query: {
source: 'object',
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this 'object' source at all? Is it entirely specific to the query behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need something to distinguish query( 'div', html() ) and query( 'div', { a: html(), b: prop() } and it's not easy to avoid ambiquity between an object defining thee shape of the sub sources or an object defining a regular source.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I see the issue. I don't think we want to natively support the concept of sub-sources, but rather enable specific sources like query to encapsulate the behavior they need. In other words, I'd really prefer we not have object be a proper source. Will poke at this a bit, since it's not obvious what a solution would be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking about this more, I think this matcher can make sense even without query if you want to shape an attribute as an object composed from several sub matchers.

Anyway, I'm open to suggestions here.

Copy link
Member

Choose a reason for hiding this comment

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

In that case, it still doesn't seem to follow that the source is an object (at least this is how it reads when seeing source: 'object'), but rather the desired shape is an object of a particular structure. The feature itself is not one I think we need to support at this point either.

(Will take a closer look shortly for more concrete suggestions)

Copy link
Member

Choose a reason for hiding this comment

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

honestly, I still think the current approach is the best approach (consistent and unambiguous)

Personally I find the object source to be very ambiguous when considered not just within the context of the query source.

The examples I'm seeing where we'd need to change the structure for a flattened query array are: pullquote, quote, and text columns, correct? And because of how they interact with Editable? Are these not the same types of blocks we'd be porting to use proper block nesting? Wondering if we're optimizing for a problem that won't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, these are the concerned blocks. I didn't consider quotes as container for paragraph blocks but I guess you're right, it's a good fit.

So what's the "temporary" option we should go with right now? removing the non-object query and transforming the value in edit/save for these blocks?

Copy link
Member

Choose a reason for hiding this comment

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

So what's the "temporary" option we should go with right now? removing the non-object query and transforming the value in edit/save for these blocks?

I'd probably be okay with this, yes. Motivation to get progress on nested blocks 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated in a4c2982

Copy link
Member

Choose a reason for hiding this comment

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

Yes, quotes would be containers for paragraph blocks, and potentially support a few others, like lists, etc.

}

/**
* Given a blocktype, a block's raw content and the commentAttributes returns the attribute value depending on its source definition
Copy link
Member

Choose a reason for hiding this comment

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

Description here doesn't seem accurate to the arguments accepted (specifically block type).

* @param {string} rawContent Block's raw content
* @param {Object} commentAttributes Block's comment attributes
*
* @return {mixed} Attribute value
Copy link
Member

Choose a reason for hiding this comment

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

From what I can tell, there is no mixed type for JSDoc, but rather the * (any) type:

http://usejsdoc.org/tags-param.html#multiple-types-and-repeatable-parameters

@@ -102,7 +102,7 @@ export function getCommentAttributes( allAttributes, blockType ) {
}

// Ignore values sources from content and post meta
if ( attributeSchema.source || attributeSchema.meta ) {
if ( attributeSchema.source !== 'comment' ) {
Copy link
Member

Choose a reason for hiding this comment

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

Will this change cause meta attributes to be serialized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no this function should only take care of comment attributes, so better be explicit about it.

switch ( attributeSchema.source ) {
case 'meta':
break;
case 'comment':
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, we've tried to avoid being explicit about comment as a source, treating it as a default. Here instead we treat default as though source is one of the hpq matchers. Maybe instead we should include the cases for those sources in matcherFromSource and let comment be the default case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, we've tried to avoid being explicit about comment as a source

Why? I see value in leaving the matchers as the non-explicit ones to avoid listing all the matchers.

It makes sense to me to have a clear matcherFromSource function responsible only for sourcing attributes from the content.

Copy link
Member

Choose a reason for hiding this comment

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

Why? I see value in leaving the matchers as the non-explicit ones to avoid listing all the matchers.

Implementation aside, it's if we conceptually consider serialized comments to be the default storage for attributes, but support other strategies for sourcing these values.

Copy link
Member

Choose a reason for hiding this comment

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

To this point, we ought to consider consistency with how we refer to the commentAttributes argument here. In createBlockWithFallback, this is named attributes. In parseWithGrammar, the block node property is named attrs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think I should use attrs in all these places?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not a huge fan of abbreviations. If we were to think of comment attributes as the "base" attribute set, then attributes would be appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's confusing to me. The function is called getBlockAttributes, it shouldn't have attributes as an argument don't you think?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is confusing, I agree. This ties into disjointedness of our attribute sourcing generally (https://github.com/WordPress/gutenberg/pull/2854/files#r144836758). What are these attributes which are magically prepopulated? Are the comments a core part of what a block is, or an incidental implementation detail? In the current context, I could see parsedAttributes making sense, but our approach of passing content and comment attributes as arguments doesn't really scale well to other source types, and as implemented we are aware of all but implementing behaviors of only some types.

How capable do we want the parsing to be? I don't necessarily think we need to build in concepts of post meta and site options to the parser itself, but maybe as some extensibility pattern: either allowing additional sources to inject their values to be referenced by the parser when encountering that source type, or hooks to allow the post editor a turn when encountering the parsing or serialization behaviors of a specific source type.

Alternatively, if it's the case that our editor selector is responsible for managing the implementation of specific types (post meta), why should this not also extend to sourcing from content as well? Ultimately maybe there is a line where a blocks core knows to handle serialized and markup-sourced attributes, then allow additional handlers to be reflected later. But ideally not with built-in awareness of what those other handlers are; if we dropped case 'meta' and treated comments as the default, would it work just as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we dropped case 'meta' and treated comments as the default, would it work just as well?

Can you provide a code example, I don't understand what you're proposing here.

: either allowing additional sources to inject their values to be referenced by the parser when encountering that source type,

This won't work for site options and post meta because their value change over time, and using the parser for this is not a good option. The parser is meant for parsing post_content (HTML) and the content contains only two types of attributes: comment attributes and attributes parsed from HTML.

hooks to allow the post editor a turn when encountering the parsing or serialization behaviors of a specific source type.

Maybe, this means hooks to the getBlock selcetor and updateAttributes callback.


I think we're struggling here because we're trying to fix the symptoms instead of trying to fix the root issue. And the root issue is the Editor not being generic enough. Things would much clearer if the Editor was a generic content editor (not tied to post or site) with extensibility mechanisms.

I do think this is out of scope of the current PR

Copy link
Member

Choose a reason for hiding this comment

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

I think we're struggling here because we're trying to fix the symptoms instead of trying to fix the root issue. And the root issue is the Editor not being generic enough. Things would much clearer if the Editor was a generic content editor (not tied to post or site) with extensibility mechanisms.

This is really what I'm trying to get at in my earlier comment, and arguably it is in scope because we're introducing knowledge of a meta type to the parser with these changes that had not existed previously.


const attributes = settings.attributes
? settings.attributes
: get( window._wpBlocksAttributes, name, );
Copy link
Member

Choose a reason for hiding this comment

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

Drop trailing comma on arguments.

@@ -145,7 +145,7 @@ class VisualEditorBlock extends Component {
onChange( block.uid, attributes );

const metaAttributes = reduce( attributes, ( result, value, key ) => {
if ( type && has( type, [ 'attributes', key, 'meta' ] ) ) {
if ( type && get( type, [ 'attributes', key, 'source' ] ) === 'meta' ) {
Copy link
Member

Choose a reason for hiding this comment

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

Minor unrelated note: type && is redundant here (get will handle the falsey type)

@aduth
Copy link
Member

aduth commented Oct 13, 2017

There are failing tests in block registration.

@youknowriad
Copy link
Contributor Author

There are failing tests in block registration.

Yep, I didn't take care of tests and documentation yet, wanted to leave this after we settle on the format. I can do so now.

@codecov
Copy link

codecov bot commented Oct 16, 2017

Codecov Report

Merging #2854 into master will decrease coverage by 0.24%.
The diff coverage is 71.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2854      +/-   ##
==========================================
- Coverage   34.61%   34.36%   -0.25%     
==========================================
  Files         261      261              
  Lines        6769     7007     +238     
  Branches     1231     1317      +86     
==========================================
+ Hits         2343     2408      +65     
- Misses       3733     3837     +104     
- Partials      693      762      +69
Impacted Files Coverage Δ
blocks/library/button/index.js 9.3% <ø> (-1.81%) ⬇️
blocks/library/code/index.js 50% <ø> (-7.15%) ⬇️
blocks/library/image/index.js 50% <ø> (-2.39%) ⬇️
blocks/library/shortcode/index.js 40% <ø> (-10%) ⬇️
blocks/library/html/index.js 16.66% <ø> (-6.42%) ⬇️
blocks/library/cover-image/index.js 30.43% <ø> (-2.9%) ⬇️
blocks/library/paragraph/index.js 29.16% <ø> (-2.84%) ⬇️
blocks/library/list/index.js 6.89% <ø> (-1.06%) ⬇️
blocks/hooks/anchor.js 80% <ø> (ø) ⬆️
blocks/library/preformatted/index.js 44.44% <ø> (-5.56%) ⬇️
... and 24 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b56f09c...2aeb51a. Read the comment docs.

@youknowriad youknowriad force-pushed the try/declarative-attributese branch from 0a1b070 to ad04011 Compare October 16, 2017 09:13
@youknowriad
Copy link
Contributor Author

Updated this PR with unit tests and documentation, it's in a better shape now.

@youknowriad youknowriad removed the [Status] In Progress Tracking issues with work in progress label Oct 16, 2017
@aduth
Copy link
Member

aduth commented Oct 16, 2017

Hit another merge conflict 😄

result[ key ] = coercedValue;
return result;
}, {} );
const blockAttributes = keys( blockType.attributes ).reduce( ( memo, attributeKey ) => {
Copy link
Member

Choose a reason for hiding this comment

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

Lodash's reduce could make this a little more concise:

const blockAttributes = reduce( blockType.attributes, ( memo, attributeSchema, attributeKey ) => {

const attributeSchema = blockType.attributes[ attributeKey ];
memo[ attributeKey ] = getBlockAttribute( attributeKey, attributeSchema, rawContent, attributes );
return memo;
}, {} ) || {};
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the || {} would ever be triggered? Even if the reduce operates on an empty set, the initial value should be returned.

> [].reduce( () => {}, {} );
{}

...settings,
name,
attributes: keys( attributes ).reduce( ( memo, attributeKey ) => {
Copy link
Member

Choose a reason for hiding this comment

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

From your previous comment:

I see value in leaving the matchers as the non-explicit ones to avoid listing all the matchers.

Is the whole existence of this reduce here to support being able to specify the comment case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes?

export function getBlockAttribute( attributeKey, attributeSchema, rawContent, commentAttributes ) {
let value;
switch ( attributeSchema.source ) {
case 'meta':
Copy link
Member

Choose a reason for hiding this comment

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

Our approach for defining other attribute sources is a bit awkward, I think, in that we have to explicitly bypass the getter here to allow it to be handled elsewhere later. Would like if these strategies were more consolidated. cc @mcsf

switch ( attributeSchema.source ) {
case 'meta':
break;
case 'comment':
Copy link
Member

Choose a reason for hiding this comment

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

To this point, we ought to consider consistency with how we refer to the commentAttributes argument here. In createBlockWithFallback, this is named attributes. In parseWithGrammar, the block node property is named attrs.

source: 'query',
selector: 'div.wp-block-gallery figure.blocks-gallery-image img',
query: {
source: 'object',
Copy link
Member

Choose a reason for hiding this comment

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

A few ideas:

  • We could support one or the other, but not both, where query can either return an array of objects shaped of a particular structure, or an array of a particular source value
  • We could split the two options by property name, e.g. queryObject (array of sourced object shape) and query
  • We could test the presence of query.source and infer if it's defining a shape or the nested source

@youknowriad youknowriad force-pushed the try/declarative-attributese branch from f0aa05f to 456f054 Compare November 10, 2017 08:48
@youknowriad
Copy link
Contributor Author

A final review for this?

aduth
aduth previously requested changes Nov 15, 2017
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Not sure if it's introduced by this pull request or an issue that's been resolved since last rebase, but I'm seeing many invalid blocks when simply saving Demo and refreshing the page.


// If the block supports anchor, parse the id
if ( blockType.supportAnchor ) {
blockAttributes.anchor = hpqParse( rawContent, attr( '*', 'id' ) );
}

// If the block supports a custom className parse it
if ( blockType.className !== false && attributes && attributes.className ) {
if ( false !== blockType.className && attributes && attributes.className ) {
Copy link
Member

Choose a reason for hiding this comment

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

Thinking you might have had to make these changes only because you were running an older version of the branch with newer dependencies? (Related: #3187)

Noting that this received no response. It is not a blocker to me for merge.

@@ -102,7 +102,7 @@ export function getCommentAttributes( allAttributes, blockType ) {
}

// Ignore values sources from content and post meta
Copy link
Member

Choose a reason for hiding this comment

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

We should update this comment.

@youknowriad youknowriad force-pushed the try/declarative-attributese branch from 104a32a to 207d794 Compare November 15, 2017 16:55
@youknowriad youknowriad dismissed aduth’s stale review November 15, 2017 16:56

Rebased this and addressed the comments. I'm not seeing the "invalid" issue, it might have been fixed by the rebase.


return blocks[ name ] = settings;
return blocks[ name ] = block;
Copy link
Member

Choose a reason for hiding this comment

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

This assignment seems unnecessary given let block = blocks[ name ] = { above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch

name,
attributes: get( window._wpBlocksAttributes, name ),

const attributes = settings.attributes ?
Copy link
Member

Choose a reason for hiding this comment

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

Was the change to extract this out necessary? Is it just for providing the default to get, and if so, could that have been added in-place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason, just a rebase leftover

@youknowriad youknowriad force-pushed the try/declarative-attributese branch from 207d794 to c013766 Compare November 16, 2017 07:46
*/
import * as source from './source';

export { source };
export { createBlock, switchToBlockType, createReusableBlock } from './factory';
export { default as parse, getSourcedAttributes } from './parser';

Choose a reason for hiding this comment

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

Bug here: getSourcedAttributes was removed from parser

Choose a reason for hiding this comment

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

See PR with fix here:
#3582

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants