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 subkey option to connect #944

Closed
wants to merge 3 commits into from
Closed

Conversation

theJian
Copy link
Contributor

@theJian theJian commented May 4, 2018

This PR added an option subkey to connectAdvanced.

Currently, the createProvider accepted the second optional argument subKey, It seems like has no real usage for now. Providing the custom subscription key only to Provider is not sufficient for using this feature, connect components need to know the custom subscription key to access the subscription corresponding to custom store. By adding a new option subKey, connect can be able to access the specific subscription.

  • Add an option subKey to connectAdvanced
  • Add tests to Provider
  • Delete extra whitespaces / Identing lines in codebase.

@theJian
Copy link
Contributor Author

theJian commented May 4, 2018

Btw I would like to know if it's possible to add .editorconfig and .prettierrc.json to this project. For a consistent code style. I found those config files does exist in reactjs/redux. Thanks.

@timdorr
Copy link
Member

timdorr commented May 4, 2018

I'd argue we can just get rid of subKey. It doesn't get you anything. You know the subscription key because it follows a specific pattern. It's just extra complexity without any benefits.

@theJian
Copy link
Contributor Author

theJian commented May 4, 2018

@timdorr That's a good point. I will send a pr to remove the subKey another day. We can close this one now.

@theJian theJian closed this May 4, 2018
@jimbolla
Copy link
Contributor

jimbolla commented May 4, 2018

Yeah I don't like subKey, the sub and the store are supposed to go together. I would've made them a single thing if there was a way to do it that was backwards compatible.

@theJian theJian mentioned this pull request May 5, 2018
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