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.

Sign up for GitHub

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

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions packages/blocks/src/api/serializer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,14 @@
/**
* External dependencies
*/
import { isEmpty, reduce, isObject, castArray, startsWith } from 'lodash';
import {
isEmpty,
reduce,
isObject,
castArray,
startsWith,
isEqual,
} from 'lodash';

/**
* WordPress dependencies
Expand Down Expand Up @@ -219,7 +226,7 @@ export function getCommentAttributes( blockType, attributes ) {
// Ignore default value.
if (
'default' in attributeSchema &&
attributeSchema.default === value
isEqual( attributeSchema.default, value )
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

All checks are green @youknowriad 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the proposal, regardless!

) {
return accumulator;
}
Expand Down
22 changes: 22 additions & 0 deletions packages/blocks/src/api/test/serializer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,28 @@ describe( 'block serializer', () => {

expect( attributes ).toEqual( { fruit: 'bananas' } );
} );

it( 'should skip attributes of type "object" whose values are equal to the default value of the attribute', () => {
const attributes = getCommentAttributes(
{
attributes: {
fruit: {
type: 'string',
},
ripeness: {
type: 'object',
default: {},
},
},
},
{
fruit: 'apples',
ripeness: {},
}
);

expect( attributes ).toEqual( { fruit: 'apples' } );
} );
} );

describe( 'serializeAttributes()', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/e2e-tests/fixtures/blocks/core__gallery__deprecated-2.serialized.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
<!-- wp:gallery {"ids":[],"columns":2,"linkTo":"none","align":"wide"} -->
<!-- wp:gallery {"columns":2,"linkTo":"none","align":"wide"} -->
<figure class="wp-block-gallery alignwide columns-2 is-cropped"><ul class="blocks-gallery-grid"><li class="blocks-gallery-item"><figure><img src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAACklEQVR4nGMAAQAABQABDQottAAAAABJRU5ErkJggg==" alt="title" data-id="1" class="wp-image-1"/></figure></li><li class="blocks-gallery-item"><figure><img src="data:image/jpeg;base64,/9j/2wBDAAMCAgICAgMCAgIDAwMDBAYEBAQEBAgGBgUGCQgKCgkICQkKDA8MCgsOCwkJDRENDg8QEBEQCgwSExIQEw8QEBD/yQALCAABAAEBAREA/8wABgAQEAX/2gAIAQEAAD8A0s8g/9k=" alt="title" data-id="2" class="wp-image-2"/></figure></li></ul></figure>
<!-- /wp:gallery -->