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

Use cloneDeep from lodash when cloning a block in wp.blocks.cloneBlock #13476

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@avillegasn
Copy link
Contributor

avillegasn commented Jan 24, 2019

Description

wp.blocks.cloneBlock( block ) creates a shallow clone of the block parameter. Therefore, the attributes of the cloned block, when objects, are a reference to the equivalent in the original block. Changes on these attributes in the cloned block affect also the original block, which is something we should avoid by using a deep clone function, like lodash.cloneDeep().

How has this been tested?

Using cloneDeep from lodash inside wp.blocks.cloneBlock on blocks with attributes that are objects (i.e. 'core/table') now returns a real clone.

Types of changes

Instead of just creating the clone copying the attributes of the original block, apply the lodash.cloneDeep() function to these attributes.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@avillegasn

This comment has been minimized.

Copy link
Contributor Author

avillegasn commented Jan 24, 2019

Commands to reproduce the problem with wp.blocks.cloneBlock using a shallow clone instead of a deep clone:

  1. Create a 'core/table' block:
    let a = wp.blocks.createBlock( 'core/table', { 'hasFixedLayout':false, 'head':[], 'body':[{'cells':[{'content':'a','tag':'td'},{'content':'b','tag':'td'}]},{'cells':[{'content':'c','tag':'td'},{'content':'d','tag':'td'}]}],'foot':[] } );

  2. Clone the block:
    let b = wp.blocks.cloneBlock( a );

  3. Change attribute value in clone:
    b.attributes.body[0].cells[0].content = 'another value';

  4. Show original attribute value... it was indirectly changed:
    console.log( a.attributes.body[0].cells[0].content ); // shows "another value" instead of "a"

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 24, 2019

Thanks for the PR, It seems reasonable to me. cc @aduth ?

Would you mind adding a unit test?

@aduth

aduth approved these changes Jan 24, 2019

Copy link
Member

aduth left a comment

I think it's reasonable too that the clone be done deeply.

That said, you should definitely not be mutating attribute values directly, and this will almost certainly result in other buggy behavior. Always use setAttributes to change a value.

I considered whether something like Lodash's _.merge would be more optimized, but I don't think it has the behavior we'd want: Namely, the mergeAttributes should replace top-level keys, not merge recursively.

Fine with it as-is, but feel free to consider the suggestion posed in the review.

@@ -94,8 +95,8 @@ export function cloneBlock( block, mergeAttributes = {}, newInnerBlocks ) {
...block,
clientId,
attributes: {
...block.attributes,
...mergeAttributes,
...cloneDeep( block.attributes ),

This comment has been minimized.

@aduth

aduth Jan 24, 2019

Member

I don't expect this function would be called with too much frequency, but there's an optimization opportunity in that, if we are working with a cloned copy of the original attributes, we can be fine with the mutating behavior of Object.assign on the target argument.

attributes: Object.assign(
	cloneDeep( block.attributes ),
	cloneDeep( mergeAttributes )
),

Where the implementation here would otherwise be more akin to:

attributes: Object.assign(
	{},
	cloneDeep( block.attributes ),
	cloneDeep( mergeAttributes )
),

There's a small bit of overhead by copying properties of the cloned block.attributes into a fresh object.

...mergeAttributes,
},
attributes: Object.assign(
{},

This comment has been minimized.

@aduth

aduth Jan 24, 2019

Member

Sorry, I may have miscommunicated. This line is not necessary at all, which is the main optimization over what you had previously.

Suggested change
{},
@aduth

This comment has been minimized.

Copy link
Member

aduth commented Jan 24, 2019

A remaining thought here is that this would introduce an inconsistency between the treatment of attributes of a cloned block, and that of the default value of an attribute schema (which is not cloned).

For example, consider:

wp.blocks.createBlock( 'core/table' ).attributes.body === wp.blocks.getBlockType( 'core/table' ).attributes.body.default
// true

One could argue that this too should be provided as a copy for each block, but the original implementation was based on the assumption that you never mutate attributes directly.

See previously: #10890

In reflection, I'm actually a bit reluctant on making this change.

I think the clone should happen in your original steps where you're trying to modify the value

Change attribute value in clone:
b.attributes.body[0].cells[0].content = 'another value';

Instead, you should do something like:

const nextBody = cloneDeep( b.attributes.body );
nextBody[0].cells[0].content = 'another value';
setAttributes( { body: nextBody } );

Per discussion in #10890, I think another thing which could help surface these issues sooner to developers is to Object.freeze (deep-freeze) the attributes object, at least in development environments.

@avillegasn

This comment has been minimized.

Copy link
Contributor Author

avillegasn commented Jan 25, 2019

Thanks for the explanation. Now I understand that attributes shouldn't be accessed directly to modify them. The thing is that in my scenario I can't use the setAttributes function (unless I'm wrong) because I'm not inside the edit function of a block.

My intention is to duplicate the currently selected block and translate its contents (i.e. its attributes) to another language, and then insert the block with the translated content. Since I don't have the possibility of using setAttributes, what I plan to do now that I know that you never mutate attributes directly is the following:

Instead of creating the block as follows, what I really do is obtaining the currently selected block
by using let a = wp.data.select( 'core/editor' ).getSelectedBlock();
but here I use the createBlock just for you to know how the block that I'm working with is:

let a = wp.blocks.createBlock( 'core/table', { 'hasFixedLayout':false, 'head':[], 'body':[{'cells':[{'content':'Hola','tag':'td'},{'content':'b','tag':'td'}]},{'cells':[{'content':'c','tag':'td'},{'content':'d','tag':'td'}]}],'foot':[] } );

Then I clone the block and (following your explanation) its attributes, to modify them.

let b = wp.blocks.cloneBlock( a );
let attributes = lodash.cloneDeep( b.attributes );
attributes.body[0].cells[0].content = 'Hello'; // translation of 'Hola'

In order to update the attributes of block b (the clone), I need to first insert it and then use the updateBlockAttributes from the editor.

wp.data.dispatch( 'core/editor' ).insertBlocks( [a, b] );
wp.data.dispatch( 'core/editor' ).updateBlockAttributes( b.clientId, attributes );

What is weird to me here is that I need to first insert the clone in the editor and then update its attributes using updateBlockAttributes that way. I expected a generic function called setAttributes or updateAttributes to exist within the block object itself, but apparently it does not.

Should that function really exist? Am I doing something wrong?

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Jan 25, 2019

Thanks for explaining, I see the challenge you're posing.

Maybe I shouldn't take as harsh a stance on mutating the values, or at least as far as the values produced from the blocks module. A block object could reasonably have its properties reassigned, when not operating in the context of what's held in the editor state.

So, in your example, I might say that instead of calling to update the attributes, you assign a new attributes property on the block object with the mutated clone.

let b = wp.blocks.cloneBlock( a );
let attributes = lodash.cloneDeep( b.attributes );
attributes.body[0].cells[0].content = 'Hello'; // translation of 'Hola'
b.attributes = attributes;
wp.data.dispatch( 'core/editor' ).insertBlocks( [a, b] );

Or, to avoid any mutation:

let b = wp.blocks.cloneBlock( a );
let attributes = lodash.cloneDeep( b.attributes );
attributes.body[0].cells[0].content = 'Hello'; // translation of 'Hola'
b = { ...b, attributes };

I could see how this might contradict my previous statements. The main issue we see with mutating attributes directly is from, as you mention, an edit function, where not only does the change not take place immediately (or at least, it doesn't force that it should), it revises any reference to the block, particularly through state history (e.g. Undo/Redo will break).

The values produced from the blocks modules are outside of this history and component lifecycle, so are generally more okay to manipulate directly.

That still brings us to the original issue, which is that blocks produced by cloneBlock can retain references to either the original block, or the block definition's default attribute values.

I may be open to the idea of letting the cloning take effect deeply for what occurs within the blocks module, but I think it should at least be done consistently, i.e. also applying to default values.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 26, 2019

I may be open to the idea of letting the cloning take effect deeply for what occurs within the blocks module, but I think it should at least be done consistently, i.e. also applying to default values.

Applying to the default values would break some assumptions we made in different parts of the editor (isUnmodifiedDefaultBlock for instance) which I'm not sure if we want to change (performance...).

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Jan 28, 2019

Related: #7252

I'm not sure if we want to change (performance...).

Good point. It seems while there is some convenience to it, the disadvantages (and perhaps inconsistency, at least in messaging) could outweigh it.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 29, 2019

Thanks for your contribution @avillegasn. Based on the discussions above, I'm going to close this PR for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.