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

Clone serverWatchSet to not modify it #6085

Closed
wants to merge 1 commit into from
Closed

Conversation

mitar
Copy link
Contributor

@mitar mitar commented Jan 26, 2016

Otherwise settingsWatchSet is added to bundleResult.serverWatchSet which is at the same also cachedServerWatchSet.

Also a fix for style.

@mitar mitar mentioned this pull request Jan 27, 2016
@mitar
Copy link
Contributor Author

mitar commented Apr 15, 2016

Ping.

@benjamn
Copy link
Contributor

benjamn commented May 10, 2016

This seems to interfere with the assumptions of this code below:

if (! canRefreshClient) {
  // Restart server on client changes if we can't refresh the client.
  serverWatchSet = combinedWatchSetForBundleResult(bundleResult);
}

Specifically, if ! canRefreshClient, we throw away the cloned serverWatchSet you created above, which effectively discards the settingsWatchSet files you added to it.

Does that make sense? Is that cause for concern?

@mitar
Copy link
Contributor Author

mitar commented May 11, 2016

Maybe then serverWatchSet.merge(settingsWatchSet); should be called after ! canRefreshClient.

So what worries me is that you modify something which I see as input (serverWatchSet) internally.

But probably it does not matter. Feel free to close this pull request.

@stubailo
Copy link
Contributor

But probably it does not matter. Feel free to close this pull request.

Doing this.

@stubailo stubailo closed this May 24, 2016
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.

3 participants