(WIP) Ignore deprecation messages for non-core-migrated preferences in the deprecation proxy #58145
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fullofcaffeine
changed the title
|
Size Change: -3 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
fullofcaffeine
changed the title
…not in core/edit-post or core/edit-site scopes
youknowriad
reviewed
Jan 23, 2024
| } | ||
|
|
||
| // Fallback to original scope for other cases | ||
| return originalGet( state, scope, name ); | ||
| }; | ||
|
|
There was a problem hiding this comment.
I don't really understand the change here. I was expecting just a removal of some keys from the array on line 7
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Closed in favor of a more straightforward approach in #58153. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What / Why / How
Follow up to (in chronological order):
corescope #58031#58031 augmented the proxy added in #58016 to make it fallback to the original scope if the setting (preference) was not found in the
corescope. However, as part of this path, a deprecation message is also output. This deprecation message ends up breaking a lot of Jest tests because @wordpress/jest-console explicitly requires deprecation messages to be expected (usingtoHaveWarnedWith.At first, I thought about "fixing" those by ignoring those deprecation messages in therelease/17.5branch, but after a discussion with @youknowriad, he correctly pointed out that a better 17.5-specific solution would be to just fallback to theoriginalGetcall for any of the preferences that were not refactored to thecorescope inrelease/17.5, which this PR aims to do.Actually, a more pragmatic solution would be to skip the deprecation message if:
getcall was asking for the preference in theedit-siteoredit-postscope;corescope.This means that the preference has not been migrated yet to
corein Gutenberg, and a deprecation message should not be shown.Testing Instructions
All checks should pass.