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 rtype interface for initialFeatures to Readme. #25

Closed
kennylavender opened this issue Aug 17, 2017 · 4 comments · Fixed by #36
Closed

Add rtype interface for initialFeatures to Readme. #25

kennylavender opened this issue Aug 17, 2017 · 4 comments · Fixed by #36
Assignees

Comments

@kennylavender
Copy link
Contributor

kennylavender commented Aug 17, 2017

For #16.

I am unsure how to describe initialFeatures in rtype, so I am experimenting here.

// example object
const initialFeatures = {
  'comments': {
    enabled: false,
    dependencies: []
  },
  'user-ratings': {
    enabled: false,
    dependencies: ['comments']
  }
}
interface Feature {
  enabled: Boolean,
  dependencies: Array
}
interface IntialFeatures {
  [featureName: String]: Feature
}
@kennylavender kennylavender self-assigned this Aug 17, 2017
@kennylavender kennylavender mentioned this issue Aug 17, 2017
9 tasks
@kennylavender
Copy link
Contributor Author

kennylavender commented Aug 17, 2017

I think I have the above signatures correct, they feel a bit odd to me though. I think they feel odd because the Feature interface does not have a name, and the rest of the properties are useless without a name.

I wonder if we should make the signatures and implementation more intuitive with something like

const initialFeatures = [{
    name: 'comments',
    enabled: false,
    dependencies: []
  },{
    name: 'user-ratings',
    enabled: false,
    dependencies: ['comments']
  }];
interface Feature {
  name: String,
  enabled?: false,
  dependencies?: [...featureName: String]
}
const initialFeatures = [ ...Feature]

@kennylavender
Copy link
Contributor Author

@ericelliott @thoragio thoughts?

@thoragio
Copy link
Contributor

I think the second way is clearer, and it also addresses my array or object question by using an array to hold the list of features, which is more intuitive IMO.

@kennylavender
Copy link
Contributor Author

@thoragio cool thanks, I think I might make a pr that changes the implementation to this

kennylavender added a commit that referenced this issue Aug 29, 2017
[Fixes #25] Change Feature interface
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 a pull request may close this issue.

2 participants