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

Add syncWindows option #299

Closed
wants to merge 1 commit into from
Closed

Add syncWindows option #299

wants to merge 1 commit into from

Conversation

richard-viney
Copy link

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:

  • Is enableWindowSync the best name for this?
  • 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.
  • Currently this is a global option, not a per-storage option. I think this is fine initially.
  • 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.

Thanks.

@fsmanuel
Copy link
Member

@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
Copy link
Author

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 richard-viney changed the title FEATURE - Add enableWindowSync option Add syncWindows option Apr 19, 2019
@richard-viney
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants