Page MenuHomePhabricator

Keys used to store user settings in local storage do not follow one naming convention
Closed, DeclinedPublic

Description

Popups keep two user settings in the browser local storage:

  • is feature enabled for anonymous user under mwe-popups-enabled key
  • preview count under ext.popups.core.previewCount key

Source: https://github.com/wikimedia/mediawiki-extensions-Popups/blob/d07441ec7f419915a6c984ae0f244c2ba5596c71/src/userSettings.js#L11

The isEnabled is following the old naming convention, preview count new one. We should follow only one naming convention and migrate data from mwe-popups-enabled to ext.popups.core.enabled key.

Questions

Both values are stored as a separate object, can we use a one object that keeps both variables?

Event Timeline

pmiazga created this task.Aug 15 2017, 8:36 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 15 2017, 8:36 PM

Any reasons for it to be one particular way? Given preview count will go away when EventLogging is removed do we need to do anything here or can we wait until that happens?

Migrating localStorage keys usually involves 2 steps:

  • Backwards compatible change that removes the previous key from users localstorage
  • Follow up change to remove the migration code.

(otherwise we pollute user storage unnecessarily which may be restricted in size..)

If we're going to drop the EventLogging then no action is required. I do not have reasons to pick the one way, but I want to make naming consistent across all data stored in localstorage.

Jdlrobson closed this task as Declined.Aug 23 2017, 4:06 PM

Declined in favor of T173952