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

Move a few components to es6 with mixin decorator #2439

Merged
merged 1 commit into from
Dec 8, 2015
Merged

Move a few components to es6 with mixin decorator #2439

merged 1 commit into from
Dec 8, 2015

Conversation

rob0rt
Copy link
Contributor

@rob0rt rob0rt commented Dec 8, 2015

This is a redirection of #2436 to its own branch.

@alitaheri alitaheri added this to the ES6 Classes milestone Dec 8, 2015
@alitaheri
Copy link
Member

awaiting a conclusion on #2437.

@oliviertassinari
Copy link
Member

@subjectix I think that we can merge this one.
However, the rebase after v0.14 isn't going to be straightforward if we continue migrating the doc,
and adding descriptions to getDefaultProps.

@alitaheri
Copy link
Member

@oliviertassinari No need to worry we'll iteratively merge master with this branch to keep it updated.


@autobind
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use arrow functions instead of autobind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of the arrow functions I could alternatively just leave react-codemod alone; it automatically puts this bindings in the constructor. Which would you prefer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a look at https://github.com/callemall/material-ui/pull/1734/files, I would say, let's let react-codemod puts this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry but I completely disagree! Doing that makes the components extremely hard to maintain! I had one hell of bad time with leftnav's doc page component. This is not a good pattern. and using arrow would make it look like this:

_foo = () => {
  // ... 
};

It's not so bad, is it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@subjectix react-router seems to follow this convention too (https://github.com/rackt/react-router/blob/master/examples/huge-apps/components/GlobalNav.js).
I don't know why they and facebook do so, but I feel like we can trust them.

@rob0rt
Copy link
Contributor Author

rob0rt commented Dec 8, 2015

I just updated with a version that leaves react-codemod's this binding alone.

@alitaheri
Copy link
Member

I seriously disagree with this binding -_-

This makes maintenance a living hell. please don't go down this path 🙏 🙏

@oliviertassinari
Copy link
Member

This makes maintenance a living hell.

I agree with you.
However, I think that there is a good reason why react-router and facebook are following this path.
Any clue?

@oliviertassinari oliviertassinari mentioned this pull request Dec 8, 2015
@alitaheri
Copy link
Member

@oliviertassinari I can think of one thing: named functions, they help debugging. but that's not a priority with a library like this! this library changes very fast. Maintainability is far more important that some tiny extra information in the stack trace. I use them with my project and I'm truly happy with them.

@rob0rt
Copy link
Contributor Author

rob0rt commented Dec 8, 2015

I personally like the @autobind syntax since I think it's a good middle ground between syntax and maintainability.

es7 stage-1 decorators are still pretty new so I'm not surprised a ton of libraries aren't using them yet, though I have found uses in the wild.

Personally, I use them for my React project (as well as other decorators, unrelated) and I find them to be easy to understand and keep the function declaration syntax consistent.

@rob0rt
Copy link
Contributor Author

rob0rt commented Dec 8, 2015

But in the end I will defer to whatever the collaborators decide on.

@alitaheri
Copy link
Member

@BobertForever Go on and use @autobind if decorators are accepted then we can party 🎉 🎉. if not. we'll change them back to arrow functions with gallons of tears :D :D
But for now, keep the productivity up 👍 👍

@rob0rt
Copy link
Contributor Author

rob0rt commented Dec 8, 2015

Sounds like a plan to me :)

(And I'll join you on the tears, I have so many of these decorators all around my codebase at this point)

@rob0rt
Copy link
Contributor Author

rob0rt commented Dec 8, 2015

Just updated the diff back to using the decorator

@alitaheri
Copy link
Member

I'm happy with this 👍 Thanks a lot @BobertForever 🎉

@oliviertassinari Please merge if it's ok.

@oliviertassinari
Copy link
Member

@BobertForever Thanks for working on this.

oliviertassinari added a commit that referenced this pull request Dec 8, 2015
Move a few components to es6 with mixin decorator
@oliviertassinari oliviertassinari merged commit 165bf2b into mui:es6-classes Dec 8, 2015
@alitaheri
Copy link
Member

🎉 🎉 Our first shot at this 👍

@zannager zannager added the package: system Specific to @mui/system label Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: system Specific to @mui/system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants