funkensturm / ember-local-storage

The addon provides a storageFor computed property that returns a proxy and persists the changes to localStorage or sessionStorage. It ships with an ember-data adapter.
https://www.funkensturm.com/ember-local-storage/
MIT License
218 stars 76 forks source link

Add syncWindows option #299

Closed richard-viney closed 1 month ago

richard-viney commented 5 years ago

Adds an enableWindowSync option so that changes to values in local storage can optionally not immediately propagate to other windows/tabs.

Addresses #298.

I've kept the diff small initially. A few discussion points:

Thanks.

fsmanuel commented 5 years ago

@richard-viney that looks great!

I have a quick question about the reason you need that feature. Your current implementation doesn't take the adapter indices into account. They will still sync.

Is enableWindowSync the best name for this?

That is a good question. Other options could be syncWindows or just sync.

Now that the global config has another option, the config section in README.md could be revised a bit to better match other addons. I can do this if desired.

I'm totally open for a revised version!

Currently this is a global option, not a per-storage option. I think this is fine initially.

Agreed! For a per-storage option we need to do something similar to the initialState() definition.

Is the way that the config value is being extracted and passed to the storage object reasonable? There's a bit of duplication in _isWindowSyncEnabled(). We could have a more formal 'config defaults' object and merge it with what the user provides instead. I can do this if desired.

I think we can have a _addonConfig function:

function _addonConfig() {
  let appConfig = getOwner(context).resolveRegistration('config:environment');
  let addonConfig = appConfig && appConfig['ember-local-storage'] || {};

  return addonConfig;
}

Thanks for all the work!

richard-viney commented 5 years ago

I'm not aware of the role of adapter indices - what do they do?

I'm using this in an app that stores some UI state in local storage, e.g. is a specific sidebar opened/closed. When multiple copies of the app are open syncing means that a change in one window propagates to another, which isn't desirable.

I've pushed a rename of the option to syncWindows and tidied that addon config part.

richard-viney commented 5 years ago

Have squashed and rebased against 1.7.0.

My team is no longer using this add-on, having opted for an in-house decorator-based solution that avoids needing to use get(). I still think this feature is worth adding though.

Re the first reply above, I'm still not aware of the role of adapter indices and their potential relevance to the changes in this PR.