Zoom Out: Remove unlock() from getSettings #64141
Conversation
…ly locked associated with the useZoomOut hook and feature.
tjcafferkey
added
[Type] Bug
|
Size Change: -10 B (0%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in f5e4bf6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10182256602
|
|
I think it is the right thing to do. I also did similar in this PR - #64073. I didn't expect there would be more instances, so didn't check. 😄 From what I know, there is a kind of private setting, though there are only two keys specified (neither are gutenberg/packages/block-editor/src/store/private-actions.js Lines 14 to 58 in ff59ffa It also looks like the settings aren't locked, just that it might strip out the private keys. |
|
I think it might be incorrectly locked. P.S. It seems that we have a couple of methods for making object value access private; we should probably settle on one. |
That's interesting. I wonder why the errors happen. I guess I should revert my PR, as it might have caused some issues. I also wonder why this is an editor setting compared to a private selector. It looks like the section client id is already derived from store state: |
I'm unsure about that, but it seems a good candidate for the private selector. Maybe @ajlende or @scruffian can clarify the intention behind making it a setting. |
|
I'm also concerned about how this locking works when the editor settings are updated via the reducer as it creates a new object (this is where I was looking for signs of locking): gutenberg/packages/block-editor/src/store/reducer.js Lines 1653 to 1677 in 30c6dfb |
|
@tjcafferkey, I think we might want to change how this setting is exposed. We should probably avoid adding it as a setting and create a new private selector that derives But let's see what others have to say. |
I think this does work ok as the locking data is reference via a key/value to the data, so it'll be retained in the new object. #64073 was reverted btw. @tjcafferkey Do you have more info about how to repro the warnings/errors you're seeing? One possible issue is that the locking of settings happens in the |
I believe this happens when you use the To provide testing instructions I'd likely have to use Edit: Apologies for the late reply here, was tied up with other things this week! |
|
I've been AFK for a (long) while so sorry not to have inputted here sooner.
My understanding (cc @scruffian and @MaggieCabrera who were here when I was away) is that The Site Editor uses the The Based on the above I don't think we can move the As to the locking element - I can't say, but I assume folks were trying to keep it private for now. |
Could you have a private action for setting the |
@talldan Sorry I realised I didn't fully address your point. The reason we can't put that logic directly in block editor, despite it being derived for interrogating the blocks in the editor, is that some of these blocks are specific to the WP editors ( |
Yes I was thinking about that just now. I think technically it's possible. I'm just wondering if we chose the settings root for a reason or whether it was just that alternatives weren't considered. I'm thinking about the benefits of that approach:
Is there anything else I'm missing? I'll have a go at implementing this approach in a separate PR. |
|
Some initial work here #64544 |
This PR is currently a WIP. I'm not sure if removing
unlockis the correct thing to do as of yet.What?
Remove
unlock()around the usage ofgetSettings()Why?
This causes an error since it wasn't locked and surfaces when using the
useZoomOuthook.How?
Remove
unlock()from the necessary parts of the codebase associated with the error.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast