Fix an issue with the serializer not ignoring attribute values of object attribute type when the value is equal to the default.

[kmgalanakis]

Description

When an attribute is of type object, if its value is manipulated and ends up empty, it is always saved in the database no matter if the attribute has an empty object as a default value or not. Changing the way of comparing the actual value of the attribute with its default value, during attribute serialization can resolve the issue. Checking with === always returns false for {} === {} for known reason. Switching to isEqual seems to resolve the issue.

How has this been tested?

Manual testing and unit tests.

Types of changes

This is a bug fix obviously.

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.
• I've updated all React Native files affected by any refactorings/renamings in this PR.

[gziolo]

This change seems reasonable in terms of consistency. It's quite important now that Global Styles promote objects as default values in the official documentation. I'm wondering if it there are any considerations that make this proposal problematic in terms of backward compatibility.

[kmgalanakis]

@gziolo do you think there is something missing in this PR? What can I do to move this forward? Thank you.

[youknowriad]

isEqual is avoided for performance reasons I believe. How are you "resetting" the default value, you could be using the same instance?

That said, if there's no measurable impact, I'm fine with this change personally.

[kmgalanakis]

I know that there might be performance "issues" when using isEqual but in that specific case, I don't expect any significant ones because it will just compare the value with a default one which is not expected to be that complex. So IMO there will be no measurable impart.

Regarding your other question, I'm sorry, but I'm not really following.

[youknowriad]

How are you empetting the value in your block. Basically you should be able to do something like setAttributes( { myAttribute: myBlocksSettings.attributes.myAttribute.default } ) to reset the value and don't suffer from this issue.

[kmgalanakis]

OK, now I get it. To reset the value in my block I had to do something like setAttributes( { myAttribute: {} } ), since the default value for my attribute was an empty object, and it was enough for my specific case.

Before this change, even though my attribute value matched the default value, it was always saved in the post content no matter what.

[youknowriad]

I'm not against merging this to make it simpler for block authors but it looks like there's some test failures?

[kmgalanakis]

You were right but it should be good to go now. The e2e tests are failing randomly. I don't know what to do there.

[kmgalanakis]

All checks are green @youknowriad 😄

[mcsf]

I feel reluctant about this change. Switching from strict equality to any kind of rich comparison seems like a dangerous precedent to set, muddying attribute semantics and introducing more unexpected situations than it fixes.

Yes, performance is one concern, but even if there were no measurable performance impact from this change (for which we can't just rely on the performance tests, since this is not the kind of operation that they target) I would rather not merge this.

I concede that "resetting" an attribute of type 'object' can be confusing for block developers, but it's also a direct consequence of the flexibility and strangeness of object types in JS. But, as much as we like to mock JS's type relationships, I think JS is right about the semantics of objects in one regard: an empty object {} should be different from "no value" or "default value". Developers should retain the ability to set an attribute's value to an explicit {} value without the framework cleverly coercing that value into nothingness.

Thus, the practice of unsetting an attribute explicitly by assigning schema[attributeName].default or undefined to it should remain the standard, and my recommendation is to improve the documentation to reflect this.

[kmgalanakis]

Understood and fine for me. I've found a way to do my job anyway. Thanks for the review.

[mcsf]

Thanks for the proposal, regardless!