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.

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

Data: Implement preferences persistence using user settings #15800

Closed
wants to merge 1 commit into from

Conversation

aduth
Copy link
Member

@aduth aduth commented May 23, 2019

Closes #15105

This pull request seeks to explore a (likely non-viable) approach for preferences persistence, leveraging the user settings cookie, existing utils script methods for operating on this cookie, and substitute storage implementation for the data persistence plugin.

Problems:

  • setUserSettings explicitly forbids (and removes) non-ASCII characters from values, so storing a stringified JSON blob is not directly feasible (source)
    • Technically, we could bypass this by assigning directly to the cookie
  • Per above, cookie storage limits are quite small. It varies by browser, but is apparently roughly considered capped at 4kb (reference). Anecdotally, my own data persistence preference as a string sizes about 1kb. This obviously does not account for (a) other user settings and (b) future growth.

For this reason, it may not be advisable to consider the cookie for storage, though there is an appeal in trying to reuse an existing feature targeted at an identical goal (client-side UI settings).

Alternative considerations will continue to be proposed in and linking to #14512.

storage: {
getItem: function() {
return (
window.getUserSetting( 'dataState' ) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

How complex it would be to transform this to a package. To be clear, I think the current proposal is good enough, just wondering if we should take this opportunity to packagify this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just saw the size limitations, it's unfortunate. What's the alternative you're thinking of? an endpoint?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I like how nice this solution is. I'd give it a try and see how it scales :)

@gziolo
Copy link
Member

gziolo commented May 24, 2019

  • Per above, cookie storage limits are quite small. It varies by browser, but is apparently roughly considered capped at 4kb (reference). Anecdotally, my own data persistence preference as a string sizes about 1kb. This obviously does not account for (a) other user settings and (b) future growth.

Try to disable all blocks using Block Manager and check again 😅 I know that I'm not helping here but this might be an actual issue. We still could try to go with this proposal as it solves the problem for the majority of cases.

);
},
setItem: function( key, value ) {
window.setUserSetting( 'dataState', value );
Copy link
Member

@gziolo gziolo May 24, 2019

Choose a reason for hiding this comment

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

The source code of the method:

https://github.com/WordPress/wordpress-develop/blob/8347d7e22f98520228309392757ea75fd7bca4db/src/js/_enqueues/lib/cookies.js

// Both name and value must be only ASCII letters, numbers or underscore
// and the shorter, the better (cookies can store maximum 4KB). Not suitable to store text.

@aduth
Copy link
Member Author

aduth commented May 24, 2019

I should have been clearer in my original comment: I have little faith that this implementation is viable. Cookie limitations are likely a blocker.

It's presented here both as (a) a showcase at an attempt to reuse existing functionality and (b) to solicit suggestions which would allow for cookies to be used, considering the noted limitations.

I expect it would probably be closed and we move to an alternative implementation, likely using user meta, server-side inline scripts to "upsert" the persisted value into localStorage, and continue to use the localStorage mechanism, syncing from the client on change using user endpoint meta fields (related comment: #15105 (comment)).

@jorgefilipecosta
Copy link
Member

jorgefilipecosta commented Jun 28, 2019

Hi @aduth, It seems there is a decision already taken not to follow this approach. But as another point doesn't this approach suffers from the same problems as the local storage? E.g.: if the user has some extension than cleans the cookies all the time will not the preference be ignored? Settings will also not be synchronized across devices right?

@aduth
Copy link
Member Author

aduth commented Jul 1, 2019

@jorgefilipecosta On the surface, it would appear that it would, but the cookie is used more as a transportation mechanism. Both the server and the client are aware of the cookie, and the server will persist changes to the database. If the user loses the cookie for whatever reason (new browser, clear storage), the server will also restore it.

It works quite well, but the main blocker are the storage limitations on cookies, which are much smaller than we can probably tolerate.

@aduth
Copy link
Member Author

aduth commented Nov 4, 2019

Closing as non-viable. Discussion for the original issue should continue in #15105.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Data /packages/data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Persist user's editor preferences to database rather than local storage
4 participants