Add new preferences persistence API, and save editor preferences in user meta #39795
Conversation
|
Size Change: -616 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
|
The more I'm working on this, the more I'm thinking that the ideal setup will be two packages:
This would entail moving the react components that are already in |
packages/wp-preferences/src/database-persistence-layer/index.js
Outdated
Show resolved
Hide resolved
talldan
force-pushed
the
try/preferences-configurable-persistence-layer
branch
from
bc432e3 to
6498c0e
Compare
|
@youknowriad This is now working using an inline script, but there might be a better way of doing it (see my two comments above). Seems to work pretty well in usage though. We can also preload the preferences via the script, which might be a safer option compared to relying on the api fetch preloading. |
| $preload_data = get_user_meta( $user_id, $meta_key, true ); | ||
|
|
||
| wp_add_inline_script( | ||
| 'wp-database-persistence-layer', |
There was a problem hiding this comment.
I don't understand why you chose to add this to wp-database-persistence-layer instead of wp-preferences (which mean you don't have to add any dependencies to any other script than wp-preferences)
There was a problem hiding this comment.
Hmm, true. I'm still learning about the best way to do this. Thanks for the pointers.
talldan
force-pushed
the
try/preferences-configurable-persistence-layer
branch
from
5e42c97 to
218d002
Compare
| @@ -0,0 +1,3 @@ | |||
| # Database persistence layer | |||
There was a problem hiding this comment.
Thinking about the name here:
- Should the name include "preferences" to clarify that it's about persisting preferences
- Should the name include "api" instead of "database" (I think that's probably arguable)?
There was a problem hiding this comment.
Should the name include "preferences" to clarify that it's about persisting preferences
I'd like to add data migration code to the package that's specific to preferences, so I think it makes sense to mention preferences.
A problem with api and database is that those names are about the implementation. If we wanted to change it in future we'd probably need a new package.
We could go with something like preferences-persistence-layer and in the README state that this is the persistence layer for WordPress preferences. Maybe it exports createUserMetaPersistenceLayer, and if in the future we want to persist preferences in a different way, a new function can be added and the old one can be deprecated.
There was a problem hiding this comment.
the problem with preferences-persistence-layer is that it a local storage is also a persistence layer. I'm fine keeping "database" or "server" or something like that.
There was a problem hiding this comment.
A problem with api and database is that those names are about the implementation. If we wanted to change it in future we'd probably need a new package.
It's hard to imagine that this implementation would change for WordPress in the future. Anyway, based on the keywords used in the code and your discussion I wanted to check what do you think about the following ideas:
@wordpress/user-preferences@wordpress/preferences-user-persistence
There was a problem hiding this comment.
I quite like those two suggestions @gziolo. Do you have a preference @youknowriad?
There was a problem hiding this comment.
What about just @wordpress/preferences-persistence ?
There was a problem hiding this comment.
My 2¢ on naming:
Agree with Riad's thinking. There's two things to name.
The first thing is the abstraction we're introducing to @wordpress/preferences. You've chosen persistence layer. I think it might be clearer to call this something like preferences backend or preferences store. Using the word preferences makes it clear that @wordpress/preferences is what we're talking about.
The second thing is the type of the above abstraction that puts preferences into the database. You've chosen database which I think is a little misleading because it really uses the REST API. I doubt that will ever change.
So I'd go with rest-api-preferences-store or something along those lines.
BUT all the names suggested so far are a bit weird when you consider that this new package also contains all of the migrations for legacy local storage preferences (src/migrations/legacy-local-storage-data). Maybe that stuff should move to to the package where the legacy calls to localStorage used to live? (@wordpress/editor I think?)
There was a problem hiding this comment.
You've chosen database which I think is a little misleading because it really uses the REST API. I doubt that will ever change.
I'm not so sure about that, a switch to GraphQL, websockets, or webRTC could all make that name irrelevant. It would be a very big change for WordPress, but then using a realtime communication protocol is very likely to become a reality for collaborative editing. While it's unlikely everything would go through such an API, I think it's enough of a reason not to consider 'REST API' in the naming.
It's this kind of reason that most other packages are named for what they do rather than how they do it. There are exceptions, but most of those are very small utils. Because of that I would favour something like preferences-persistence or preferences-user-persistence.
I think I'll go with preferences-persistence as Riad suggested, just to keep it simple.
talldan
force-pushed
the
try/preferences-configurable-persistence-layer
branch
from
21000ec to
aa1a2bf
Compare
|
I think the reason why a lot of the tests have been failing is due to the way the page reloads after toggling a preference here in the gutenberg/packages/e2e-test-utils/src/create-new-post.js Lines 46 to 53 in 88689f0 I think that's preventing the API request that would persist the change to preferences. After the page reloads it's like An option could be to use Or maybe we just accept that this may happen and make the tests more robust. 🤔 edit: Another option still could be to use local storage as well as a fallback layer. There would need to be a way to know whether local storage is more recent than the database. |
|
Still a lot of e2e tests failing. I think another reason for that might be that the tests clear gutenberg/packages/e2e-tests/config/setup-test-framework.js Lines 78 to 81 in 606788a But here the preferences are persisted between test runs in the database. |
talldan
force-pushed
the
try/preferences-configurable-persistence-layer
branch
from
6424138 to
081604a
Compare
|
Still getting a lot of test failures. There are also still some tests that do a There are other test failures that are a bit more obscure. I'm having a hard time reproducing and/or understanding those. It'd be good to get an idea of whether there are specifically bugs that are likely to happen for humans, or if these failures are only a result of the tests running so quickly. |
talldan
force-pushed
the
try/preferences-configurable-persistence-layer
branch
2 times, most recently
from
59f5dea to
70eb336
Compare
gziolo
added
the
[Type] Technical Prototype
|
I think the e2e tests are mostly passing now. The tests did need some extra setup code to clear server stored preferences before and after test runs, so that was one of the main things that was missing. Even then, solely using an async server API wasn't stable enough, there were still issues like page reloads in the tests racing and beating the preferences API request, which meant after the reload the editor state would be incorrect. These would have been edge cases for human users, but to make things more robust I've made the preferences persist to user meta and also local storage as a backup. If for some reason the user meta preferences are outdated, the local storage will be used instead. |
I feel like I had some things like that in page reloads for other stuff as well. It might good to have our own dedicated "reload" helper where we could inject things safely and not update every test at some point. (or maybe you did that I didn't check :P )
This is cool but makes me wonder if it's better to actually "preload" the preferences and make them "blocking" for the UI. Only show the UI once they're loaded. I'm thinking about folks loading their editor in a new browser and noticing a "jump" when the preferences load. Still not sure what's best though. |
@youknowriad Do you remember what the other stuff was? I did think about this option. Waiting for network requests to complete before a reload might be a start.
The good news is that they are preloaded. The issue is more that when the page reloads, a request to save preferences can be in flight and not finish before the page has reloaded. This is especially true because some tests are toggling a lot of preferences quickly (opening and closing panels and sidebars), and the server requests are debounced to avoid overwhelming the server, so the last timeout or request can be delayed or interrupted. |
I don't know but for me, it was more about doing something after the "reload". For instance I see
👍 |
|
This seems to be working well now. I've added some code to migrate existing local storage preferences to the new system. To finish up I'll add docs, tests and changelog updates. Then I'll consider splitting the PR up into smaller PRs (it is quite big, though it's mostly tests at the moment). There's also a need to consider the naming of the new package, so I'll put some more thought into that. |
|
Is it possible to disable persisting preferences in DB for some cases? I noticed that an API request is triggered every time switch between a Document and a block. CleanShot.2022-04-28.at.10.29.13.mp4 |
|
@Mamaduka It's a valid concern. At the moment there isn't a way to handle this. I think it would require some extra configuration for particular preferences. A possibility is that some preferences don't trigger a request. They could save to local storage, and maybe they do persist to user meta too, but they could only update when another preference is saved AND/OR only update after a particular period of time AND/OR only update on browser unload (it's difficult to ensure this works though). |
|
I was thinking about a similar method. For example, we could use post autosave to trigger preference syncing with user meta. |
Interesting, I wonder if that's the reason for the performance gain (now that we don't use localstorage which might have been synchronous) Anyway, it does feel like switching between sidebars shouldn't be a preference at all. Whether the sidebar is closed or open should be, so there's a small refactoring to be done here to improve things. |
|
I opened #40715 with minor edits to the package configuration. |
Yeah agree this doesn't need to be persisted across sessions. |
I've made a PR that does this - #40923 |
|
There may be a few other things we also want to refactor in this way. Expanded/collapsed panels in the post editor's document settings is something we could consider removing IMO. I also have a PR to make the block editor package's It would result in a REST API request every time a block is inserted. This might be a more challenging one to solve. |
I also noticed that. Maybe API can support a list of preferences that don't need to be persisted immediately. |
| sprintf( | ||
| '( function() { | ||
| var serverData = %s; | ||
| var userId = "%s"; |
There was a problem hiding this comment.
Minor and belated, but it seems safer to encode userId formally, in the same way as serverData, even if we know that the result
sprintf( 'userId = %s', wp_json_encode( $user_id ) );would be identifical to the current form
sprintf( 'userId = "%s"', $user_id );The former doesn't rely on our own mental type checking, and redundancy helps with safety — especially around code interpolation. :)
talldan
added
the
Needs Dev Note
Adds a new feature to persist editor UI preferences between page loads and browsers. * Adds a new preferences persistence API. * Saves editor preferences in user meta instead of in browser's local storage. Why? Due to the transient nature of browser storage, this persistence is not as sticky as it is expected to be, including: switching browsers (unique storage between browsers), or using private browsing tabs (storage cleared between sessions), or the same user across a network of sites (storage unique by domain). This is a backport from Gutenberg.[WordPress/gutenberg#39795 See WordPress/gutenberg PR 39795]. Props talldanwp, youknowriad, noisysocks, mamaduka, costdev, ironprogrammer, hellofromTonya. See #56467. Built from https://develop.svn.wordpress.org/trunk@54182 git-svn-id: http://core.svn.wordpress.org/trunk@53741 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Adds a new feature to persist editor UI preferences between page loads and browsers. * Adds a new preferences persistence API. * Saves editor preferences in user meta instead of in browser's local storage. Why? Due to the transient nature of browser storage, this persistence is not as sticky as it is expected to be, including: switching browsers (unique storage between browsers), or using private browsing tabs (storage cleared between sessions), or the same user across a network of sites (storage unique by domain). This is a backport from Gutenberg.[WordPress/gutenberg#39795 See WordPress/gutenberg PR 39795]. Props talldanwp, youknowriad, noisysocks, mamaduka, costdev, ironprogrammer, hellofromTonya. See #56467. Built from https://develop.svn.wordpress.org/trunk@54182 git-svn-id: https://core.svn.wordpress.org/trunk@53741 1a063a9b-81f0-0310-95a4-ce76da25c4cd
|
@talldan Curious, why doesn’t this use the existing user settings API in WordPress ( |
|
@swissspidy There was a past attempt to use user settings, which discusses some of the reasons - #15800. |
Adds a new feature to persist editor UI preferences between page loads and browsers. * Adds a new preferences persistence API. * Saves editor preferences in user meta instead of in browser's local storage. Why? Due to the transient nature of browser storage, this persistence is not as sticky as it is expected to be, including: switching browsers (unique storage between browsers), or using private browsing tabs (storage cleared between sessions), or the same user across a network of sites (storage unique by domain). This is a backport from Gutenberg.[WordPress/gutenberg#39795 See WordPress/gutenberg PR 39795]. Props talldanwp, youknowriad, noisysocks, mamaduka, costdev, ironprogrammer, hellofromTonya. See #56467. Built from https://develop.svn.wordpress.org/trunk@54182
Adds a new feature to persist editor UI preferences between page loads and browsers. * Adds a new preferences persistence API. * Saves editor preferences in user meta instead of in browser's local storage. Why? Due to the transient nature of browser storage, this persistence is not as sticky as it is expected to be, including: switching browsers (unique storage between browsers), or using private browsing tabs (storage cleared between sessions), or the same user across a network of sites (storage unique by domain). This is a backport from Gutenberg.[WordPress/gutenberg#39795 See WordPress/gutenberg PR 39795]. Props talldanwp, youknowriad, noisysocks, mamaduka, costdev, ironprogrammer, hellofromTonya. See #56467. git-svn-id: https://develop.svn.wordpress.org/trunk@54182 602fd350-edb4-49c9-b593-d223f7449a82
|
I just filed this bug report. I don't think this code should be making API calls to save user preferences if the user is not logged in. |
Related - #31965
Also related - #39672 (comment)
Fixes #15105
Fixes #34405
What?
This PR explores moving away from the current
@wordpress/dataway of persisting store data to local storage.It implements a new API in the preferences package for configuring how the store state is persisted.
It adds a new package that persists preferences to the WordPress database, as part of the user's meta.
And then finally this is all configured using an inline script.
Why?
A good reason is that this will finally allow #15105 to be closed, which is a long standing problem for users.
There are also some technical advantages that this will unlock:
@wordpress/dataregisterStorein@wordpress/datain favor ofregisterHow?
Adds a new
setPersistenceLayeraction to the preferences package. A higher order reducer is then used to keep state in sync with the persistence layer in the preferences store.There's also a new
preferences-persistencepackage (based on #19177). This saves data to user meta via the REST API primarily. It also saves data to local storage as a back in case the user goes offline or a request is interrupted.Finally, I've moved all the migration code over from the old package to this new package, any old preferences should be retained.
Testing Instructions