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

Discussion: Consider using @connect #1367

Closed
davidnguyen179 opened this Issue Dec 20, 2016 · 15 comments

Comments

Projects
None yet
8 participants
@davidnguyen179
Contributor

davidnguyen179 commented Dec 20, 2016

Would you guys consider using

@connect(mapStateToProps, mapDispatchToProps) instead of using

export default connect(mapStateToProps, mapDispatchToProps)(...); ?

It looks more clearly and more easy to understand

@gihrig gihrig added the discussion label Dec 20, 2016

@gihrig

This comment has been minimized.

Member

gihrig commented Dec 20, 2016

I like the @ decorator approach though I've never worked with it in JS (I come from a Java background where this is common).

@nndung179 I think the big issues here will be lack of familiarity in the community and complexity of implementation. Can you speak to these issues from personal experience?

@davidnguyen179

This comment has been minimized.

Contributor

davidnguyen179 commented Dec 21, 2016

hi @gihrig !

To set up the decorator is not hard at all. Things to do:

  • install babel plugin
  • then change syntax of connect

props:

  • the code looks less messy & easy to understand.
  • we can get familiar with decorator in js

cons:

  • with developers who not familiar with decorator. It must be looked strange at the first time but it is not so hard to understand.

I tried in my project. It all works fine. If PR welcome, I will make one.

@kopax

This comment has been minimized.

kopax commented Dec 21, 2016

I would love to see annotations in general to this project.

@oliverturner

This comment has been minimized.

Collaborator

oliverturner commented Dec 21, 2016

Personally I prefer the simplicity and explicitness of
export default connect(mapStateToProps, mapDispatchToProps)(...)

Agree with @gihrig that familiarity within the community should be considered when looking at changes that are a matter of individual aesthetic preference.

At the very least it will be a departure from the syntax used in most Redux examples and add a further layer of indirection for newcomers.

@redgenie

This comment has been minimized.

redgenie commented Dec 21, 2016

I'm with oliverturner. Magic is nice, but too much and it becomes confusing. Some stuff still needs to be explicitly defined.

@kopax

This comment has been minimized.

kopax commented Dec 21, 2016

@oliverturner @redgenie

If you look at others js "framework", they all stick to the standards of writing JS.

The main advantage would be to be the first to implement annotation widely on an application.
Today I can't find any helpful JS framework like Spring or Zen for building large scale application.

I can see everyday numbers of discussion and PR here but It has to take it's own way at some point, some other framework are coming out everyday with lot's of cool features.

This project earn today a lot from it's community and could take one more step to affirm it's "opinionated way" to craft JS App in 2017 by adding usage of annotations.

This is not a question to keep the code understandable to everyone, but to have a good way to write clean and maintainable code. If I need to reed a complex documentation to do that I vote for.

@oliverturner

This comment has been minimized.

Collaborator

oliverturner commented Dec 21, 2016

@kopax

The main advantage would be to be the first to implement annotation widely on an application.

I'm not sure I agree that this would be an advantage to the project. I get that you may benefit from an example of best practice, but that's the kind of trade-off that needs more than mere early-adopter status for it to be persuasive.

This project [...] could take one more step to affirm it's "opinionated way" to craft JS App in 2017 by adding usage of annotations.

I think most people would say it's already plenty opinionated!

It's great that you're comfortable reading documentation to help writing clean and maintainable code... but you have your own apps to do that in, no? Whether RBP has additional syntactic sugar doesn't affect that.

@kopax

This comment has been minimized.

kopax commented Dec 21, 2016

Whether RBP has additional syntactic sugar doesn't affect that.

I wonder why they decided to implement that then : https://en.wikipedia.org/wiki/Java_annotation , I am curious.

I would be glad to see a gist of your implementation @nndung179 to see what that sugar look like.

@Dattaya

This comment has been minimized.

Member

Dattaya commented Dec 21, 2016

I'm completely in support of decorators, but only when they are more stable (reach level 3) and released officially by Babel (more details on this: babel/babel#2645 (comment)). My two concerns are:

  1. Don't want to add another not that necessary dependency in package.json: babel-plugin-transform-decorators-legacy
  2. I don't know how well babel-plugin-transform-decorators-legacy's supported, this issue: Decorator incorrectly applies to nested class if used before export default was left unanswered, we definitely don't want any new bugs introduced because of some nice syntactic sugar.
@davidnguyen179

This comment has been minimized.

Contributor

davidnguyen179 commented Dec 21, 2016

Hi @Dattaya !

I completely agree with you. Hope it is stable soon. Thank you for the feedbacks :)

@scf4

This comment has been minimized.

scf4 commented Dec 21, 2016

I'm not sure "familiarity" is a good argument against. This boilerplate introduced me to a lot of new (and good) concepts.

@connect looks a lot better and the concept is very simple.

But wait to see how the proposal pans out first.

@davidnguyen179

This comment has been minimized.

Contributor

davidnguyen179 commented Dec 22, 2016

@kopax the reason I changed to use @connect that the syntax is simple and clear for my team. It is more meaningful when all of connect to redux start at the beginning.

@oliverturner

This comment has been minimized.

Collaborator

oliverturner commented Dec 22, 2016

Closing for now since the evidence (thanks @Dattaya) points clearly in the direction of deferring further discussion until the Decorators proposal hits stage 3.

Thank you all for your input: I had no idea before that this pattern had so many passionate fans!

Let's revisit when the spec reaches maturity.

@mxstbr

This comment has been minimized.

Member

mxstbr commented Dec 22, 2016

We definitely don't want to use legacy decorators – that'll just confuse people once the spec is officially out and then they work differently.

@lock

This comment has been minimized.

lock bot commented May 29, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators May 29, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.