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

Smooth API rough edges #99

Closed
ericelliott opened this issue Feb 11, 2018 · 2 comments
Closed

Smooth API rough edges #99

ericelliott opened this issue Feb 11, 2018 · 2 comments
Assignees

Comments

@ericelliott
Copy link
Contributor

ericelliott commented Feb 11, 2018

As long as we're thinking about a major version bump, let's examine the rest of the public API.

getEnabled() - deprecate

getEnabled() should be getEnabledFeatures() -- we can keep the old version and log deprecation warnings when it's used.

isFeatureIncluded() - rename & refocus

This is really the centerpiece of the low-level (non-component) public API.

isFeatureIncluded has an awkward name, and only makes sense in the context of a feature set that has already been decided. Also, it's advertised signature is wrong:

// This function is curried, not reflected in the signature.
isFeatureIncluded([...Strings], String) => Boolean

Maybe it should be hasFeature():

hasFeature = (featureName: String) => Boolean

Which can be a partial application of createHasFeature():

createHasFeature = (features: [...Feature]) => (hasFeature: String => Boolean)

Or in components, could delegate to a partially applied hasFeature() which can be dynamically swapped out if the features change at runtime.

getIsEnabled()

getIsEnabled() is confusing, and should probably not be part of the public API, even if we use it internally. End-users should be using hasFeature() instead, both on the server, and in the client.

Should we deprecate the publicly exposed version?

React components

In our React component props/context, do we need to expose anything other than a hasFeature() function that can be dynamically updated as feature statuses change?

@ericelliott ericelliott changed the title Smooth API weirdness Smooth API rough edges Feb 11, 2018
@kennylavender
Copy link
Contributor

I agree with all of the above. Those changes should make the api much easier to reason about.

I think we could also adjust the Feature interface so that enabled becomes isEnabled. I have noticed that myself and others expect this property to be named isEnabled

interface Feature {
  name: String,
  isEnabled: false,
  dependencies?: [...featureName: String]
}

@kennylavender
Copy link
Contributor

Closing as the v2 proposal in #102 addresses these changes.

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

No branches or pull requests

2 participants