Skip to content

Conversation

@dejewi
Copy link
Contributor

@dejewi dejewi commented Jan 16, 2017

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4c5b52d on dejewi:master into 99f20e6 on mjt01:master.

@michaeltaranto
Copy link
Owner

michaeltaranto commented Jan 17, 2017

Thanks for putting this together, are you able to provide a test that fails to capture your fix? Also if you can remove the version bump and dist changes from your PR that will come through with the publishing.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 8d4602e on dejewi:master into 99f20e6 on mjt01:master.

@dejewi
Copy link
Contributor Author

dejewi commented Jan 19, 2017

Sure :)
I reverted bump version and dist changes. I also provided test.

@dejewi
Copy link
Contributor Author

dejewi commented Jan 23, 2017

Hi Michael, Did you had chance to review my last update?

@michaeltaranto
Copy link
Owner

Looks good to me, thanks for making those changes.

@michaeltaranto michaeltaranto merged commit 069ac48 into michaeltaranto:master Jan 24, 2017
michaeltaranto added a commit that referenced this pull request Jan 31, 2017
@michaeltaranto
Copy link
Owner

@dejewi I have reverted this PR in v1.6.0 based on https://github.com/mjt01/angular-feature-flags/issues/44. Having reviewed the issues raised I agree that the correct behaviour is not what was set last, rather that the local override wins. Sorry for the confusion, I'm not actively working on this module so it can be hard to build enough context to review intent thoroughly enough.

@dejewi
Copy link
Contributor Author

dejewi commented Jan 31, 2017

ohh :(
How about if I will introduce additional parameter to control overriding eg. by default featureFlags.set() will not override feature flags stored in localstorage but if we will set/or pass additional boolean parameter set on true then we will override feature flags?

@michaeltaranto
Copy link
Owner

On reflection I actually think you are using the model incorrectly (hence the roll back). The enable function is about setting a local override for what is otherwise the source of truth. The idea of setting a local override then overriding the override using set does not sound right.

You're probably after a resetAll that deletes all overrides right?

@dejewi
Copy link
Contributor Author

dejewi commented Feb 2, 2017

Generally I don't use resetAll. My use case is very simple:

  1. in config phase set default feature flags required to run application (or just in case if feature flag service is not available), eg flag ARE_FEATURE_FLAGS_LOADED
myApp.config(function(featureFlagsProvider) {
  featureFlagsProvider.setInitialFlags([
    { "key": "ARE_FEATURE_FLAGS_LOADED", "active": false, "name": "...", "description": "..." },
  ]);
});
  1. in run phase set whole the most up-to-date feature flag list (with eg. ARE_FEATURE_FLAGS_LOADED feature flag set on true)
myApp.run(function(featureFlags, $http) {
  featureFlags.set($http.get('/api/flags'));
});

And after this two steps ARE_FEATURE_FLAGS_LOADED is set on false instead of true

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