Add isRawAttribute to entity config #34388
Conversation
packages/core-data/src/actions.js
Outdated
| @@ -373,6 +373,7 @@ export const saveEntityRecord = ( | |||
| } | |||
| const entityIdKey = entity.key || DEFAULT_ENTITY_KEY; | |||
| const recordId = record[ entityIdKey ]; | |||
| const rawAttributes = entity.rawAttributes || []; | |||
There was a problem hiding this comment.
We have to keep doing || [] and rawAttributes.includes( key ) so maybe wonder a selector like isRawAttribute( state, kind, name, attributeName ) would be more convenient here.
|
Size Change: -2.05 kB (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
packages/core-data/src/selectors.js
Outdated
| * | ||
| * @return {boolean} Is the attribute raw | ||
| */ | ||
| export function isRawAttribute( state, kind, name, attribute ) { |
There was a problem hiding this comment.
Does this need to be a public selector?
There was a problem hiding this comment.
I'd prefer it to be private. The only reason why it's public is that saveEntityRecord needs to call it. How about we change the name to __unstableIsRawAttribute to fix the widgets bug, and then later remove it as a part of the larger refactoring discussed below?
Or did you just mean to remove it from core-data exports?
There was a problem hiding this comment.
Or did you just mean to remove it from
core-dataexports?
I think this is what @youknowriad meant. If we can avoid exporting it then I'd prefer that too 🙂
Personally I think it would be okay to break backwards compatibility in this case but we should note it in |
|
This is definitely better but it's all still a bit weird to me 😀 What's irking me is that The whole point of gutenberg/packages/core-data/src/selectors.js Lines 440 to 446 in b72adf8 So, I think:
|
|
I am taking a deep dive here and will post a longer comment soon. |
|
@noisysocks Good thinking! There are definitely some layers to peel here: This PR already solves a real issue with widgets. I would like to get it merged, even if the selector would be called Adding an API such as But if removing
Move most of the post-specific logic out of core-dataThis comment says it all: gutenberg/packages/core-data/src/actions.js Lines 425 to 428 in 35e16bb Core-data hosts post-specific autosaving logic, data shape logic, API response reconciliation logic and so on. This could be a good opportunity to get more generic and move some of that logic to the server. Explore getting rid of data shape juggling entirelyAs for the The API returns data such as I wonder whether this "easier" part is still true. Maybe it would actually be easier to embrace the
Inconsistent storageThere is already some confusion even within while this action gets dispatched after reloading the editor: Also, I'm curious what @youknowriad thinks |
|
Love where your head is at! 👏
Sounds good!
I'm convinced! It definitely seems that we've introduced a lot of complexity to the entire package in order to save a relatively trivial amount of complexity in I suspect there may be a performance impact caused by switching from shallow object spread to deep object merge. Just need to check for performance regressions—no big deal. |
Maybe we can just not export I agree with you all that the dealing with these "raw" properties is not very well thought. It looks as if we try to adapt to the WP API to make edits work without much thoughts about it. maybe an ideal scenario So the questions to be asked:
I'm happy for this PR to move forward to solve the widgets case, these "raw" properties sound indeed as a complex problem. |
8af4747 to
37bad2e
Compare
|
@youknowriad @noisysocks I turned Mobile tests are acting up today on all PRs and commits so I think you can safely disregard them when reviewing. As for the larger discussion, let's continue in #34449 and #34450. |
37bad2e to
50a268e
Compare
…y similar, but is about including specific attributes regardless of their "raw-ness"
f135792 to
b28f032
Compare
Description
Solves #34335
As seen in #34335, core-data always assumes that entity records fields with a
rawattribute are always "flattened" bygetRawEntityRecord.This behavior is useful for posts:
content.rawin a post or a page refers to HTML with block markup as opposed tocontent.renderedwhich is the HTML without block markup.Unfortunately, for other entity records such as widgets, this behavior is undesired.
To remedy the problem, this PR adds an explicit entity configuration field called
rawAttributeswhich may be used to specify which fields should be treated as block markup HTML.The downside of this approach is a BC break so I wonder if a safer solution here would be to do a "denylist" instead of an "allowlist" (e.g.
nonRawAttributes).How has this been tested?
/wp-admin/widgets.php), update a single widget, click "Update", confirm that the batch request only contained a single child request related to the updated widget (as opposed to one per widget).Types of changes
New feature (non-breaking change which adds functionality)