Editor: Merge meta attributes in getEditedPostAttribute #12331
Conversation
aduth
added
[Type] Bug
| } | ||
|
|
||
| if ( ! mergeCache.has( edits[ attributeName ] ) ) { | ||
| mergeCache.set( |
There was a problem hiding this comment.
I'm not certain I understand how this cache gets invalidated when the edited value or the persisted value change?
There was a problem hiding this comment.
I'm not certain I understand how this cache gets invalidated when the edited value or the persisted value change?
It doesn't need to be. Given the nature of a WeakMap, invalidation occurs automatically by garbage collection as soon as the key is no longer referenced (i.e. as soon as it becomes replaced in state by another value).
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Keyed_collections#WeakMap_object
There was a problem hiding this comment.
Gotcha. Shouldn't we account for the currentPost value as well? I guess the reasoning is if it changes, the edited value would have changed as well?
There was a problem hiding this comment.
Gotcha. Shouldn't we account for the
currentPostvalue as well? I guess the reasoning is if it changes, the edited value would have changed as well?
That's a good point, and I don't think we could rely on that assumption.
I'm starting to wonder if we should bother caching here at all. It's not something which would have much impact on the stock editor, but could have a negative impact on integrations which make heavy use of getEditedPostAttribute( 'meta' ). An alternative may be to just use createSelector as we normally do, but I worry this would have a negative performance impact on stock editor usage.
There was a problem hiding this comment.
I won't bother caching for now. If we ever need a performant version we could create a getEditedPostMeta( metaKey )
|
Thanks @aduth, I will try to test this with my plugins tomorrow! |
| @@ -174,6 +174,7 @@ describe( 'selectors', () => { | |||
| setFreeformContentHandlerName( 'core/test-freeform' ); | |||
|
|
|||
| cachedSelectors.forEach( ( { clear } ) => clear() ); | |||
| invoke( getEditedPostAttribute.mergeCache, [ 'clear' ] ); | |||
There was a problem hiding this comment.
I guess this doesn't exist:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakMap/clear
Given how the tests are written, it might not really be necessary to clear the cache. Alternatively, we could manually assign into getEditedPostAttribute a new WeakMap in beforeEach.
|
I removed caching in 9781bcd . The additional tests (now removed) introduced in b9c8bfa can serve as a target if future caching is reintroduced. |
| // derivation. Alternatively, introduce a new selector for meta lookup. | ||
| return { | ||
| ...edits[ attributeName ], | ||
| ...getCurrentPostAttribute( state, attributeName ), |
There was a problem hiding this comment.
Should we change the order here?
There was a problem hiding this comment.
I mean edits should override the current values?
There was a problem hiding this comment.
Also makes me wonder how we unset values, are we supposed to pass null as a value in these cases?
There was a problem hiding this comment.
Should we change the order here?
Fixed in d711318.
Also makes me wonder how we unset values, are we supposed to pass
nullas a value in these cases?
Unsetting would only occur when updating the post. It's worked this way since the beginning for any edit, so I'm not sure it's critical to change now.
I might imagine either assigning to undefined or assigning to the value in currentPost could unset the edit property.
There was a problem hiding this comment.
I might imagine either assigning to
undefinedor assigning to the value incurrentPostcould unset the edit property.
It seems like a sensible, easy enhancement to consider. I'll make an issue for it.
There was a problem hiding this comment.
I might imagine either assigning to
undefinedor assigning to the value incurrentPostcould unset the edit property.It seems like a sensible, easy enhancement to consider. I'll make an issue for it.
Fixes #12289
Regression of #10827
This pull request seeks to resolve an issue where
getEditedPostAttribute( 'meta' )would return only the edited value, not including other unedited values from the current saved post.While in #10827 we'd introduced the "merge" concept to the
editsreducer, we'd not considered to account for it being a partial fragment in the return value ofgetEditedPostAttribute. The changes here apply that behavior.Implementation notes:
I made the implementation perhaps more complicated than necessary in a pursuit of avoiding returning a new reference object from each call to
getEditedPostAttribute. Elsewhere we usecreateSelectorfor similar effect, but in this instance I thought it may be too heavy-handed given that this is a rarer use-case (not present in core code) and given the fitness ofWeakMapas a cache given the edited object value as key (since the value is guaranteed to be an object, and guaranteed to never change reference unless its value actually changes).Testing instructions:
Repeat Steps to Reproduce from #12289
Ensure unit tests pass:
cc @florianbrinkmann