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

Add __experimentalUpdateSpecifiedEntityEdits and use it on the Font Library update button #60390

Closed
wants to merge 2 commits into from
Closed

Add __experimentalUpdateSpecifiedEntityEdits and use it on the Font Library update button #60390

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d203b52
add __experimentalUpdateSpecifiedEntityEdits
matiasbenedetto Apr 2, 2024
0c7e011
use __experimentalUpdateSpecifiedEntityEdits instead of __experimenta…
matiasbenedetto Apr 2, 2024
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
53 changes: 53 additions & 0 deletions packages/core-data/src/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -798,6 +798,59 @@ export const saveEditedEntityRecord =
return await dispatch.saveEntityRecord( kind, name, record, options );
};

/**
* Action triggered to update only specified properties for the entity.
*
* @param {string} kind Kind of the entity.
* @param {string} name Name of the entity.
* @param {Object} recordId ID of the record.
* @param {Array} itemsToSave List of entity properties or property paths to save.
* @param {Object} options Saving options.
*/
export const __experimentalUpdateSpecifiedEntityEdits =
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noting that we shouldn't be adding new experimental APIs anymore, we should add private APIs instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also it's not really clear what this API offers in addition to what saveEntityRecord does?

Copy link
Contributor Author

@matiasbenedetto matiasbenedetto Apr 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

saveEntityRecord saves all the changes to the database. __experimentalUpdateSpecifiedEntityEdits saves only the specified edits.

Let's take this case as example:

  • Database has this content:
{ "a": "a"}
  • Client-side entity has:
{ "a": "a", "b":"b", "c":"c" }
  • We want to persist to the database only the following JSON without changing the client side entity:
{ "a": "a", "c":"c" }

Using saveEntityRecord

saveEntityRecord( 'root', 'post-type', 1, { "a": "a", "b":"b", "c":"c" } )

{ "a": "a", "b":"b", "c":"c" } is persisted to the database.

Using __experimentalUpdateSpecifiedEntityEdits:

__experimentalUpdateSpecifiedEntityEdits( 'root', 'post-type', 1, [  'c'  ] );

{ "a": "a", "c":"c" } is to the database.

Using __experimentalSaveSpecifiedEntityEdits:

__experimentalSaveSpecifiedEntityEdits( 'root', 'post-type', 1, [  'c'  ] );

{"c":"c" } is to the database without changing the client-side entity.

I added these examples to the PR's description.

Probably the behavior added in this PR for __experimentalUpdateSpecifiedEntityEdits was the one originally thought for __experimentalSaveSpecifiedEntityEdits. In that case, we can move the additions to that function instead of adding a new one.

Copy link
Contributor

Choose a reason for hiding this comment

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

__experimentalUpdateSpecifiedEntityEdits( 'root', 'post-type', 1, [ 'c' ] );

But in this case, can't we just send saveEntityRecord( 'root', 'post-type', 1, { "c":"c" } ) directly? In other words, can't we retrieve the edits that we want to save and just pass them (or not even mark them as edits). I'm pretty sure we do this already in other places already.

Copy link
Contributor

Choose a reason for hiding this comment

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

__experimentalSaveSpecifiedEntityEdits to be honest I don't know much about this function, I need to check the history.

Copy link
Contributor

Choose a reason for hiding this comment

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

This would create a new post revision only with { "c":"c" } content in the database instead of the desired { "a": "a", "c":"c" }.

I guess that it's possible that it's an endpoint issue then because if I'm not wrong all of the endpoints in Core work as "PATCH" (maybe not the global styles one, we need to check)

Copy link
Contributor

Choose a reason for hiding this comment

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

It is also why we didn't see the issue before with __experimentalSaveSpecifiedEntityEdits (we never used it with the global styles endpoint before)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh so you're actually trying to send only settings.typography.fontFamilies and not settings. That is going to override the whole "settings" object of course. So IMO, you should just send the update settings object entirely.

The endpoints don't do "nested patches", only top level.

Copy link
Contributor

Choose a reason for hiding this comment

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

This would create a new post revision only with { "c":"c" } content in the database instead of the desired { "a": "a", "c":"c" }.

I don't think this is true for top level keys.

Copy link
Contributor

Choose a reason for hiding this comment

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

This would create a new post revision only with { "c":"c" } content in the database instead of the desired { "a": "a", "c":"c" }.

I don't think this is true for top level keys.

That's correct in my testing for the global-styles endpoint, and true generally across the REST API. styles are preserved here when a font is activated because it's a different top level key, but settings are reset (without this PR) because they're empty except for settings.typography.fontFamilies.

( kind, name, recordId, itemsToSave, options ) =>
async ( { select, dispatch } ) => {
if ( ! select.hasEditsForEntityRecord( kind, name, recordId ) ) {
return;
}
const edits = select.getEntityRecordNonTransientEdits(
kind,
name,
recordId
);

// Get the base record from the datase to apply the edits to.
const base = select.getEntityRecord( kind, name, recordId ) || {};

// Use the base record as the starting point for the edits.
const editsToSave = base;
Comment on lines +822 to +826
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Get the base record from the datase to apply the edits to.
const base = select.getEntityRecord( kind, name, recordId ) || {};
// Use the base record as the starting point for the edits.
const editsToSave = base;
// Use the base record as the starting point for the edits.
const editsToSave = select.getEntityRecord( kind, name, recordId ) || {};

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like these lines are the only difference here from __experimentalSaveSpecifiedEntityEdits.

Is there a reason that __experimentalSaveSpecifiedEntityEdits doesn't use the existing entity as a base? If not, perhaps that function could be updated rather than creating a new, mostly duplication function?

cc @getdave @ntsekouras who had PRs that used __experimentalSaveSpecifiedEntityEdits somewhat recently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that, too, I decided to create a new one to be explicit about the difference with __experimentalSaveSpecifiedEntityEdits and avoid changing the existing functionality that used by other parts of the codebase too.

Copy link
Contributor Author

@matiasbenedetto matiasbenedetto Apr 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Still, if we find that the behavior of __experimentalUpdateSpecifiedEntityEdits is the originally intended behavior of __experimentalSaveSpecifiedEntityEdits, we can add the changes from this PR to __experimentalSaveSpecifiedEntityEdits instead of creating the new function.


for ( const item of itemsToSave ) {
setNestedValue( editsToSave, item, getNestedValue( edits, item ) );
}

const configs = await dispatch( getOrLoadEntitiesConfig( kind, name ) );
const entityConfig = configs.find(
( config ) => config.kind === kind && config.name === name
);

const entityIdKey = entityConfig?.key || DEFAULT_ENTITY_KEY;

// If a record key is provided then update the existing record.
// This necessitates providing `recordKey` to saveEntityRecord as part of the
// `record` argument (here called `editsToSave`) to stop that action creating
// a new record and instead cause it to update the existing record.
if ( recordId ) {
editsToSave[ entityIdKey ] = recordId;
}
return await dispatch.saveEntityRecord(
kind,
name,
editsToSave,
options
);
};

/**
* Action triggered to save only specified properties for the entity.
*
Expand Down
11 changes: 6 additions & 5 deletions packages/edit-site/src/components/global-styles/font-library-modal/context.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,9 @@ import { toggleFont } from './utils/toggleFont';
export const FontLibraryContext = createContext( {} );

function FontLibraryProvider( { children } ) {
const { __experimentalSaveSpecifiedEntityEdits: saveSpecifiedEntityEdits } =
useDispatch( coreStore );
const {
__experimentalUpdateSpecifiedEntityEdits: updateSpecifiedEntityEdits,
} = useDispatch( coreStore );
const { globalStylesId } = useSelect( ( select ) => {
const { __experimentalGetCurrentGlobalStylesId } = select( coreStore );
return { globalStylesId: __experimentalGetCurrentGlobalStylesId() };
Expand Down Expand Up @@ -96,7 +97,7 @@ function FontLibraryProvider( { children } ) {

// Save font families to the global styles post in the database.
const saveFontFamilies = () => {
saveSpecifiedEntityEdits( 'root', 'globalStyles', globalStylesId, [
updateSpecifiedEntityEdits( 'root', 'globalStyles', globalStylesId, [
'settings.typography.fontFamilies',
] );
};
Expand Down Expand Up @@ -325,7 +326,7 @@ function FontLibraryProvider( { children } ) {
activateCustomFontFamilies( fontFamiliesToActivate );

// Save the global styles to the database.
await saveSpecifiedEntityEdits(
await updateSpecifiedEntityEdits(
'root',
'globalStyles',
globalStylesId,
Expand Down Expand Up @@ -362,7 +363,7 @@ function FontLibraryProvider( { children } ) {
if ( uninstalledFontFamily.deleted ) {
deactivateFontFamily( fontFamilyToUninstall );
// Save the global styles to the database.
await saveSpecifiedEntityEdits(
await updateSpecifiedEntityEdits(
'root',
'globalStyles',
globalStylesId,
Expand Down
Loading