Skip to content

Conversation

taion
Copy link
Contributor

@taion taion commented Oct 23, 2015

Closes #2364

Here's one way to let users easily use Lifecycle when using ES6 classes. Zero LoC change on our end!

I think this is a bit cleaner than implementing on our wrapper - it'd essentially just be duplicating code from a perfectly good library that's already there.

@taion
Copy link
Contributor Author

taion commented Oct 23, 2015

cc @brigand re: https://github.com/brigand/react-mixin#aside-do-i-need-mixins

Per #2364 (comment) I think the cleanest approach with ES7 is to apply this as a @Lifecycle decorator, but that's pretty much the same thing as just using the mixin.

Personally I don't think it's necessary or helpful for every library to really just re-implement this logic from react-mixin for creating mixin-style decorators, and anyway logically it is just a mixin, just with slightly different syntax.

I don't think that caveat in the react-mixin docs is necessarily warranted.

knowbody added a commit that referenced this pull request Oct 24, 2015
Document confirming navigation with ES6 classes
@knowbody knowbody merged commit 695bb67 into remix-run:master Oct 24, 2015
@taion taion deleted the lifecycle-mixin-es6 branch October 30, 2015 14:24
@filod
Copy link

filod commented Nov 3, 2015

@taion I lost this binding in routerWillLeave when using react-mixin
image

image

should we really use this?

@knowbody
Copy link
Contributor

knowbody commented Nov 3, 2015

I'm not a big fan of react-mixin to be honest, not entirely sure about this solution.

@taion maybe we should rethink that?

@taion
Copy link
Contributor Author

taion commented Nov 3, 2015

This is the abstraction breaking a bit because ES6 classes don't auto-bind.

I'm not hugely attached to react-mixin, but I can't really think of a better solution. We could make our own decorator, but it'd essentially be the same as just calling reactMixin.decorate(Lifecycle) code-wise, so I'm not sure if that would improve things.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants