Copy reference type default attributes when creating a new block

[talldan]

Description

This is a small fix to make sure that the default attributes for a block type cannot be mutated within a block instance's edit method.

This issue originated from a message in the #core-editor channel in slack: https://wordpress.slack.com/archives/C02QB2JS7/p1540130491000100

After some investigation I discovered that createBlock passes default objects and arrays by reference instead of copying/cloning them when createBlock is called, leading to unexpected results if those references are mutated. To make things easier for block implementors, the solution in this PR is to copy those defaults in createBlock.

How has this been tested?

• Added unit tests
• Manual testing using a custom block that mutates its attribute (there's a fuller example in the slack chat). ie:

onClick={ () => {
  testAttribute.newProperty = 'test';
  setAttribute( { testAttribute } );
} }

Then to test this custom block:

1. Add a new block of the custom type
2. Trigger the mutation of the attribute
3. Add another new block of the custom type
4. Observe that in the second block the default should not have been mutated

Screenshots

This issue can result in the behaviour where the default attribute is mutated in the first block, and then successive new blocks when added also use the mutated default:

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.

[talldan]

Not sure if we should consider deep cloning? Having said that, everything worked ok in my tests.

[noisysocks]

Not sure if deep cloning would be worth the slight performance penalty.

To simplify this if, could we use _.clone?

https://lodash.com/docs/4.17.10#clone

[talldan]

Nice! I was sure I searched for a clone in the lodash docs and couldn't find anything, but there it is, clear as day.

[gziolo]

Technically speaking, you could also verify before mutation happens the following:

expect( block.attributes.content ).not.toBe( arrayDefault );

which check whether they are equal and share the same reference.

[gziolo]

You can also use ES6 syntax, not sure if it is faster though:

defaultValue = [ ...defaultValue ];
defaultValue = { ...defaultValue }; 

but it's shorter for sure :)

[gziolo]

It looks like a legit fix. I would appreciate having confirmation from @aduth, @youknowriad or @iseulde that it doesn't bite us in other aspects. Do you think there might be other methods which need to be verified?

[aduth]

Preemptively blocking the merge to allow discussion.

My initial impression: This code is "doing it wrong" for more reason than one:

onClick={ () => {
  testAttribute.newProperty = 'test';
  setAttribute( { testAttribute } );
} }

[aduth]

typeof null === 'object'

[aduth]

I don't think we want this. Block implementers should not be mutating attributes directly, as it will have very unpredictable and buggy behavior in how attributes are referenced. It's much the same (in fact, literally the same, since it is in state) as Redux's read-only state principle:

https://redux.js.org/introduction/threeprinciples#state-is-read-only

If we want a developer experience improvement, I think we should do so by (deep-)freezing the attribute value so that it never gives any impression of working at all.

[talldan]

@aduth - I recommended a non-mutating fix to the block implementor in slack, and I agree with that concept. I did pause before creating this PR, considering whether it was necessary, and after some thought my opinion was that this is a bug.

That's purely because I don't think any actions within a created block should cause the original definition to change. I think this could be considered synonymous to a created React component somehow causing the initial declaration of the component to be modified—even though those APIs encourage non-mutation there's still a level of protection. When working within a component or a block, I'd have an expectation that changes do not leak out—a created block should potentially be its own sandbox—I could screw up that one instance by triggering a mutation, but it shouldn't affect others that haven't undergone the mutation.

I think there's also a dev experience aspect. I'm sure a lot of block builders are learning both React and Gutenberg (and possibly functional concepts) at the same time, so it might be good to offer a bit more implicit safety.

[aduth]

I think this could be considered synonymous to a created React component somehow causing the initial declaration of the component to be modified—even though those APIs encourage non-mutation there's still a level of protection.

Can you elaborate on this? What protection are you referring to?

By contrast, if you mutate a prop in a React component which had been provided through a defaultProps definition, it mutates the default.

https://codepen.io/aduth/pen/PyVXZZ

I'm sure a lot of block builders are learning both React and Gutenberg (and possibly functional concepts) at the same time, so it might be good to offer a bit more implicit safety.

But the example from the original comment still wrong and error-prone in its state-mutating nature, even with the clone behavior.

[talldan]

Can you elaborate on this? What protection are you referring to?

That potentially buggy code is contained/localized and doesn't cause issues more widespread than needed.

By contrast, if you mutate a prop in a React component which had been provided through a defaultProps definition, it mutates the default.

I'm surprised by that.

But the example from the original comment still wrong and error-prone in its state-mutating nature, even with the clone behavior.

I worry that it's not clear enough to block implementors why mutation is a problem—there's no developer feedback that the mutation is an error. Bugs caused by mutations could be quite subtle and hard to spot. I also couldn't find much (or anything) in the handbook to explain why attributes shouldn't be mutated, but I could've missed it. Maybe there's an assumption that implementors understand the paradigm from using React/Redux.

Perhaps at the very least this PR can be replaced with another one that adds more information to the docs?

[aduth]

Perhaps at the very least this PR can be replaced with another one that adds more information to the docs?

I'm fine with that. I also don't mind the idea of applying deepFreeze on attributes of created blocks to more loudly error when a mutation is attempted. It should only be applied in an environment we can guarantee to be for development (like SCRIPT_DEBUG) and unfortunately we've found that many developers don't run with these constants.

[aduth]

Also worth noting that conveniences don't make things simpler when in the broader sense we don't apply the conveniences consistently. If I receive positive or negative feedback in mutating attributes, will it alter my expectations in cases where I may be tempted to try the same (e.g. adding a new property to the return value of getPostEdits selector?). In these instances, it could be argued that we're setting people up for failure.

[noisysocks]

unfortunately we've found that many developers don't run with these constants

Getting off topic here, but I keep thinking that WordPress needs a Developer Mode switch in WP Admin which enables extra warnings.

[bfintal]

I haven't tested, but this may fix #11705

[talldan]

Just to update on this:

• Error when deprecating the default value of a block attribute #11705 wasn't related, I've put in a separate attempt at a fix for that
• I've started working on some minor documentation updates to explain why attributes shouldn't be mutated. Will close this issue once I have a PR open for that (edit - Add section to setAttributes documentation about immutability of attributes #12811 covers that).