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

Eliminate Navigation/State mixins #835

Closed
mjackson opened this issue Feb 13, 2015 · 21 comments
Closed

Eliminate Navigation/State mixins #835

mjackson opened this issue Feb 13, 2015 · 21 comments
Labels

Comments

@mjackson
Copy link
Member

It's becoming clear that mixins are not the way forward in React, so we need to get rid of the mixins we have, or at least deprecate them and recommend alternative methods. Fortunately, this won't be very difficult for us since both Router.Navigation and Router.State just delegate all methods to methods with the same name in context that are passed down from the router instance.

I propose that we move the actual router object into context. Then, when you need to make a path or do some navigation you just use this.context.router.transitionTo instead of this.transitionTo.

So this

var MyHandler = React.createClass({
  mixins: [ Router.Navigation ],
  handleSomeEvent: function () {
    this.transitionTo( ... );
  }
  // ...
});

becomes this

var MyHandler = React.createClass({
  contextTypes: {
    router: Router.PropTypes.router.isRequired
  },
  handleSomeEvent: function () {
    this.context.router.transitionTo( ... );
  }
  // ...
});

Note: This will also help us greatly slim down the number of properties we have on context (see #744)

We should also deprecate the use of Router.Navigation and Router.State and emit warnings when people use them.

@browniefed
Copy link

Does this get you steps closer to supporting ES6 and React .13? I re-read the React .13 blog post and it said nothing about supporting/not-supporting context for ES6 components.

@mjackson
Copy link
Member Author

@browniefed It definitely gets us closer to ES6, yes. I believe that mixins will still be supported for the foreseeable future in React, but we want people to be able to recommend ES6-compatible usage.

@browniefed
Copy link

@mjackson Ah I just meant continuing to use context not moving away from mixins (which is a good idea).

@gaearon
Copy link
Contributor

gaearon commented Feb 14, 2015

Context isn't going away. It's changing to be parent-based rather than owner-based but that shouldn't affect the router. (And 0.13 just deprecates old context behavior, doesn't change it yet IIRC.)

@uberllama
Copy link
Contributor

I still want to hear the React team's plan for multi inheritance going forward, if they really are going to deprecate mixins -- which is a horrible shame IMO. Composition is great and I just can't envision my current large React app without it at this point.

@gaearon
Copy link
Contributor

gaearon commented Feb 15, 2015

I still want to hear the React team's plan for multi inheritance going forward, if they really are going to deprecate mixins -- which is a horrible shame IMO. Composition is great and I just can't envision my current large React app without it at this point.

They're not deprecating mixins, just making them optional. They were always considered by React team an escape hatch. Nevertheless, even though they are not deprecated, using them less in libraries makes such libraries more usable from alternative environments such as Om, TypeScript, etc. We should strive for that.

You should subscribe to this issue if you're interested: facebook/react#1380

@gaearon
Copy link
Contributor

gaearon commented Feb 15, 2015

See also: https://twitter.com/sebmarkbage/status/565600338422792192

You might not need mixins at all if React exposes lifecycle hooks as something you can subscribe to a la Rx.

@uberllama
Copy link
Contributor

I'll check them out, cheers.

@ryanflorence
Copy link
Member

@mjackson

Dropping all our context junk on this.context.router would be fantastic.

I still want to hear the React team's plan for multi inheritance going forward

React does not want to extend JavaScript with a new way to create objects, that's why they are emphasizing the use ES6 classes, or even just plain constructors for components. This way its up to you (and the language) to compose those component definitions however you'd like.

@uberllama
Copy link
Contributor

I guess my follow up would be that I've seen very little on using context. Interested to get more... context on that. :)

@browniefed
Copy link

@uberllama yeah they haven't emphasized it because they recommended to not depend on it. But via ReactJS Conf Q&A panel, and various github issues context is here to stay. They're developing a new reconciliation algorithm to properly reconcile context.

@mjackson
Copy link
Member Author

Dropping all our context junk on this.context.router would be fantastic.

I'm working on it :)

@ericclemmons
Copy link

Glad to see some work on this front! I've been having issues with a project creating a component that wraps a custom context (much like <Handler />), and requires cloning contexts for everything to work. A single this.context.router would be great!

@uberllama
Copy link
Contributor

Target release version?

@mjackson
Copy link
Member Author

@uberllama probably next minor release. 0.13

@uberllama
Copy link
Contributor

Cheers.

@mjackson mjackson added this to the React 0.13 milestone Feb 25, 2015
@insin
Copy link
Contributor

insin commented Feb 25, 2015

Have the React devs indicated if React.createClass() is ever going to be slated for deprecation?

It seems like the sort of thing that'll always be needed for additional extension-time functionality (like mixins and hooking up multiple of the same lifecycle method to execute) because class and extends are just sugar.

@AndrewRayCode
Copy link
Contributor

How will this interact with willTransitionTo? Those methods won't have access to the router because static methods don't have access to context.

@jrunning
Copy link

I have the same question as @delvarworld. Any thoughts?

badsyntax referenced this issue in badsyntax/react-seed May 2, 2015
@zzzbj
Copy link

zzzbj commented Jan 22, 2016

var MyHandler = React.createClass({
es6:
export class MyHandler extends React.Component {

then
contextTypes: {
router: Router.PropTypes.router.isRequired
},
not work

@ericclemmons
Copy link

@zzzbj You want React.PropTypes.object.isRequired.

https://github.com/rackt/react-router/blob/v2.0.0-rc5/upgrade-guides/v2.0.0.md#lifecycle-mixin

(I've confirmed this is working as of v2.0.0-rc5)

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

No branches or pull requests

10 participants