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

(WIP) Ignore deprecation messages for non-core-migrated preferences in the deprecation proxy #58145

Closed
wants to merge 2 commits into from
Closed

(WIP) Ignore deprecation messages for non-core-migrated preferences in the deprecation proxy #58145

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f19bd17
Figure out what preferences have not been migrated in 17.5
fullofcaffeine Jan 23, 2024
a74fc8c
Refactor withDeprecatedKeys to skip deprecation warning for settings …
fullofcaffeine Jan 23, 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
28 changes: 14 additions & 14 deletions packages/preferences/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,30 +20,30 @@ const withDeprecatedKeys = ( originalGet ) => ( state, scope, name ) => {
'showListViewByDefault',
];

// Check if setting is in the list and scope is either 'core/edit-post' or 'core/edit-site'
if (
settingsToMoveToCore.includes( name ) &&
[ 'core/edit-post', 'core/edit-site' ].includes( scope )
) {
deprecated(
`wp.data.select( 'core/preferences' ).get( '${ scope }', '${ name }' )`,
{
since: '6.5',
alternative: `wp.data.select( 'core/preferences' ).get( 'core', '${ name }' )`,
}
);

const value = originalGet( state, 'core', name );

// Hotfix for 17.5. Some of the preferences in the list above haven't been
// migrated to core in 17.5 (i.e: `editorMode`, https://github.com/WordPress/gutenberg/pull/57642))
// so we should fallback to the passed scope to avoid unexpected `undefined` values.
if ( value === undefined ) {
return originalGet( state, scope, name );
// If the value is found in the 'core' scope, return it and show deprecation message
if ( value !== undefined ) {
deprecated(
`wp.data.select( 'core/preferences' ).get( '${ scope }', '${ name }' )`,
{
since: '6.5',
alternative: `wp.data.select( 'core/preferences' ).get( 'core', '${ name }' )`,
}
);

return value;
}

return originalGet( state, 'core', name );
// If the value is not found in the 'core' scope, return it from the original scope without deprecation message
}

// Fallback to original scope for other cases
return originalGet( state, scope, name );
};

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really understand the change here. I was expecting just a removal of some keys from the array on line 7

Copy link
Member Author

@fullofcaffeine fullofcaffeine Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I updated the description, I'm experimenting with the following:

Actually, a more pragmatic solution would be to skip the deprecation message if:

The original get call was asking for the preference in the edit-site or edit-post scope;
and the preference cannot be found in the core scope.

This means that the preference has not been migrated yet to core in Gutenberg, and a deprecation message should not be shown.

This basically has the same effect as removing the non-migrated preferences from the list, but it does it at runtime, ensuring the correct logic is selected for each of them just-in-time. I tried to get a list of non-migrated preferences, but the results were inconclusive. I used the following script that I ran from the browser console after the editor was loaded on the new post page:

(function() {
    const settingsToMoveToCore = [
        'allowRightClickOverrides',
        'distractionFree',
        'editorMode',
        'fixedToolbar',
        'focusMode',
        'hiddenBlockTypes',
        'inactivePanels',
        'keepCaretInsideBlock',
        'mostUsedBlocks',
        'openPanels',
        'showBlockBreadcrumbs',
        'showIconLabels',
        'showListViewByDefault',
    ];

    const { select } = wp.data;
    const preferencesStore = select('core/preferences');

    settingsToMoveToCore.forEach(setting => {
        const valueCore = preferencesStore.get('core', setting);
        const valueEditPost = preferencesStore.get('core/edit-post', setting);
        const valueEditSite = preferencesStore.get('core/edit-site', setting);

        if (valueCore === undefined) {
            if (valueEditPost !== undefined) {
                console.log(`Setting '${setting}' found in 'core/edit-post' but not migrated to 'core' yet.`);
            } else if (valueEditSite !== undefined) {
                console.log(`Setting '${setting}' found in 'core/edit-site' but not migrated to 'core' yet.`);
            } else {
                console.log(`Setting '${setting}' is undefined in all scopes.`);
            }
        } else {
            console.log(`Setting '${setting}' is present in 'core'.`);
        }
    });
})();

For release/17.5, I got the following:

Setting 'allowRightClickOverrides' is present in 'core'.
Setting 'openPanels' is present in 'core'.
Setting 'showBlockBreadcrumbs' is present in 'core'.
Setting 'showIconLabels' is present in 'core'.
Setting 'showListViewByDefault' is present in 'core'.
Setting 'fixedToolbar' is present in 'core'.
Setting 'inactivePanels' is present in 'core'.

Setting 'editorMode' found in 'core/edit-post' but not migrated to 'core' yet.
Setting 'hiddenBlockTypes' found in 'core/edit-post' but not migrated to 'core' yet.

Setting 'focusMode' is undefined in all scopes.
Setting 'distractionFree' is undefined in all scopes.
Setting 'keepCaretInsideBlock' is undefined in all scopes.
Setting 'mostUsedBlocks' is undefined in all scopes.

At first sight, it seems I'd need to remove editorMode and hiddenBlockTypes. But then, what about distractionFree, keepCareInsideBlock and mostUsedBlocks and focusMode? I suspect they are set in other code paths that have not run as part of the initial editor rendering.

Another way would be to search the code for get or set calls, but that's also error-prone.

Finally, delegating the logic to the actual proxy at runtime seems safer, as it will be decided when the get is called instead of ahead of time. There might be a small performance hit compared to the other solution, but it'd be a good tradeoff, considering it's only specific to release/17.5.

However, if you could clarify the cases above, we could remove them from the list as you originally suggested. Is it enough to remove editorMode and hiddenBlockTypes from the list? What about the other ones (distractionFree, keepCareInsideBlock and mostUsedBlocks and focusMode)?

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

undefined value might mean false so for me, it shouldn't be an indication about whether we should use one scope over the other.

I don't really understand why we're complicating things:

  • If a key has been migrated in 17.5, use the proxy for it
  • If the key has not been migrated, don't use the proxy for it.

The only difference with the trunk is the list of keys, in trunk all the keys are migrated.

Copy link
Contributor

Choose a reason for hiding this comment

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

What am I missing?

Copy link
Member Author

@fullofcaffeine fullofcaffeine Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The only difference with the trunk is the list of keys, in trunk all the keys are migrated.

Right.

if a key has been migrated in 17.5, use the proxy for it
If the key has not been migrated, don't use the proxy for it.

What would be the best way to find out what keys have/have not been migrated in release/17.5? Manually by checking the list of PRs in the branch? I wanted to avoid doing it manually as it could be error-prone (e.g I don't know what PRs were opened/merged related to this migration work).

Copy link
Member Author

@fullofcaffeine fullofcaffeine Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

undefined value might mean false so for me, it shouldn't be an indication about whether we should use one scope over the other.

Thanks for clarifying. Just to document here though, undefined was the return value that caused the BSOD in the Site Editor, which led me to work on this hotfix: #58031.

Still, the logic here shouldn't break anything if the value returned from core is undefined, as it will then fallback to the call in https://github.com/WordPress/gutenberg/pull/58145/files#diff-d90eaf55b416dc18bd23b193564ae65f7555ab87a40c0ceae54137374c6ca510R4 which should then return undefined if the setting is not in the given scope, or an actual truthy value if it is.

Copy link
Member Author

@fullofcaffeine fullofcaffeine Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What would be the best way to find out what keys have/have not been migrated in release/17.5?

It looks like the non-migrated keys in release/17.5 are editorMode and hiddenBlockTypes, see my comment in #. I don't know about the others (distractionFree, keepCareInsideBlock, mostUsedBlocks and focusMode) that are not found anywhere else (core, core/edit-post, core/edit-site), though.

Copy link
Member Author

@fullofcaffeine fullofcaffeine Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'll open another PR with those two settings removed from the list. I will keep this draft, just in case. I'll use the test results to see if there are remaining preferences that should be removed from the list.

EDIT: #58153.

Expand Down
Loading