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

withQueries decorator syntax #9

Merged
merged 1 commit into from
Oct 18, 2016
Merged

Conversation

tailsu
Copy link

@tailsu tailsu commented Oct 18, 2016

Improving on #6

withQueries is the same in spirit as, for example, withRouter from react-router. The original connect function remains semantically the same.

Added docs to the README as well as unit tests.

an alternative to using connect.
@tailsu
Copy link
Author

tailsu commented Oct 18, 2016

The PR is best viewed with whitespace changes turned off :)

https://github.com/roman01la/react-horizon/pull/9/files?w=1

@roman01la
Copy link
Owner

@tailsu I think I like how it is done in #6, where connect can be used both as decorator and as a function. What do you think?

@tailsu
Copy link
Author

tailsu commented Oct 18, 2016

I'm not sure...

@connect(...)
class MyComponent extends Component {
// ...
}

Okay, it's not that hard to grok, because there will be a bunch of subscriptions and horizon calls inside the connect(), so you can guess that it pertains to horizon. But still...

I guess I just like this @withWhatever style for HOCs that the people from react-router prefer. They have a nice discussion about it here: remix-run/react-router#3350

It's your call, I can redo the PR and overload connect instead.

@tailsu
Copy link
Author

tailsu commented Oct 18, 2016

By the way, #6 breaks the old syntax for connect, that's why the PR build fails 😼

@roman01la
Copy link
Owner

Didn't know about React Router is doing it this way. If it's a common naming for such decorators, I agree with the choice. Thanks!

@roman01la roman01la merged commit e3c82e7 into roman01la:master Oct 18, 2016
@roman01la roman01la mentioned this pull request Oct 18, 2016
@tailsu
Copy link
Author

tailsu commented Oct 18, 2016

Great! If it's not too much of a bother, could you publish the new package?

@roman01la
Copy link
Owner

Yes, sure. Released as v0.4.0

@tailsu
Copy link
Author

tailsu commented Oct 18, 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.

2 participants