Skip to content

Boatload of methods removed from State mixin #1502

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

Closed
etodanik opened this issue Jul 8, 2015 · 18 comments
Closed

Boatload of methods removed from State mixin #1502

etodanik opened this issue Jul 8, 2015 · 18 comments

Comments

@etodanik
Copy link

etodanik commented Jul 8, 2015

So, in the latest commit to the State mixin, I can see that a boatload of functionality was stripped and moved here:
https://github.com/rackt/react-router/blob/master/modules/URLUtils.js

However, what I don't see is now a way of easily doing any of it in my components. Is there a way to reach this functionality? Or has it been removed with a vague assumption that nobody wants anything but isActive() from their State mixin?

@agundermann
Copy link
Contributor

Not sure either. One reason for their removal could be that your route handlers get everything else injected as props anyway

  • State.getRoutes() => props.branch
  • State.getParams() => props.params
  • State.getPathname() => props.location.pathname
  • State.getQuery() => props.location.query
  • State.getPath() => no equivalent but can be reconstructed with pathname and query

@etodanik
Copy link
Author

etodanik commented Jul 8, 2015

Well, I'm not sure that's as useful. For instance - I have a <NavBar/> component that heavily depends on path. It feels kinda awkward injecting all the props into it.

Would have been way cleaner just having the State mixin.

@agundermann
Copy link
Contributor

I have a component that heavily depends on path.

Well, if a component depends on something, passing it via props and being explicit about it is the cleanest way imo. I'm not objected to bringing back the old mixin methods though.

@jedwards1211
Copy link

+1 for bringing back the old mixin methods.

A workaround would be pretty easy; all one has to do is make the topmost route component put the pathnames, params, query, etc. obtained from URLUtils into the getChildContext() in some form, and make a replacement for the State mixin whose methods pull these values out of this.context.

@agilgur5
Copy link

Not sure if this is the reason for it, but mixins don't exist in ES6 style classes, just to keep in mind.

@mjackson
Copy link
Member

We're generally trying to move away from mixins, so you'll have to use the props we pass to your route handler components instead. This will help users making the transition to ES6 classes (and possibly other usage paradigms, like pure functions) still be able to use the router.

@jedwards1211
Copy link

You mean you guys don't hope class mixins are incorporated into a future version of JS? I'm a little disappointed you'd give them up that easily. I did more than enough Java programming and debugging complex inheritance hierarchies, and I thought it was cool, until I got to JS and could use mixins. Besides, there are any number of alternative methods React could provide to mix methods into an ES6 class prototype.

@jedwards1211
Copy link

I've felt like React has served the community very well so far. If React takes away the convenience of things like the ReactMeteorData mixin I've been using, they'd better come up with a better alternative.

@jedwards1211
Copy link

I‘d rather see react-router take the approach that sebmarkhage (very active in the React project) advocates for them:

"However, I think we need to wait on this and not thrash this API since there is active work exploring what first-class mixins would look like in ES7+."

@gaearon
Copy link
Contributor

gaearon commented Aug 11, 2015

I‘d rather see react-router take the approach that sebmarkhage (very active in the React project) advocates for them:

"However, I think we need to wait on this and not thrash this API since there is active work exploring what first-class mixins would look like in ES7+."

He said many things :-). Some things he said later:

To be clear, mixins is an escape hatch to work around reusability limitations in the system. It’s not idiomatic React.

Making composition easier is a higher priority than making arbitrary mixins work.

I’ll focus on making composition easier so we can get rid of mixins.

I wouldn't say mixins are the future for React, especially considering work in progress on components as pure functions. You can use the mixins if you like them, but libraries like React Router should provide a solution that works for everyone, and injecting props is the React component API.

@mjackson
Copy link
Member

I'm a little disappointed you'd give them up that easily

To be clear, we are going to ship a 1.0 API that will use mixins, namely the Navigation and State mixins that are currently used in several of the examples. The majority of people are still writing React code that uses createClass (including the router) and we intend to support them. Also, many people who have used the router up to this point have used these mixins and we don't want to force everyone to change their entire codebase when we ship 1.0.

Having said that, all I'm saying here is that I don't want to introduce any more mixins because we'll be moving away from them post 1.0.

bronson referenced this issue in Khan/style-guides Oct 22, 2015
Summary:
https://docs.google.com/document/d/1i2bhSamrNs1C2RVP8T0bYcJ8SavNRAlNn6isU2_WI9w/edit

Also:
   https://docs.google.com/document/d/1QvQcinFbo2IJ104fVNRc88QYcLdVMXnvQ3ONPTDVDEE/edit

When updating the TOC, I noticed a bug in the script, which I fixed.

Test Plan:
Visited
       file:///home/csilvers/khan/style-guides/style/javascript.md
in a browser with a markdown-rendering plugin, and saw that the new
section rendered ok.

Reviewers: jared, emily, jlfwong, riley, kevinb

Reviewed By: kevinb

Differential Revision: https://phabricator.khanacademy.org/D22321
@gsdean
Copy link

gsdean commented Nov 18, 2015

To be clear, we are going to ship a 1.0 API that will use mixins, namely the Navigation and State mixins

Is this still the plan? if so, any idea when/where?

@taion
Copy link
Contributor

taion commented Nov 18, 2015

What specifically did you want from that mixin?

@gsdean
Copy link

gsdean commented Nov 18, 2015

From State getPath, getQuery, etc. From Navigation transitionTo. Basically we are upgrading from 0.13, and without a drop in replacement, it's going to be difficult.

@taion
Copy link
Contributor

taion commented Nov 18, 2015

Of course. Those are respectively context.location.pathname, context.location.query, and context.history.pushState (or just push, with a slightly different signature).

@gsdean
Copy link

gsdean commented Nov 18, 2015

Maybe I'm missing something. Are you saying that context should have those properties by default, or are you suggesting I implement getChildContext and provide them?

As far as I can tell, the this.context is empty by default...

@gsdean
Copy link

gsdean commented Nov 18, 2015

I see. Thanks for the clarification. The comment We will probably provide a higher order component that will do this for you but haven't yet was leading me to believe that wasn't supported yet.

FWIW, i wrapped it up in a mixin that I can drop in. Seems to work well.

var LegacyStateMixin = {
    contextTypes: {
        location: React.PropTypes.object
    },
    getQuery: function () {
        return this.context.location.query;
    },
    getPath:function(){
        return this.context.location.pathname;
    }
};

@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
None yet
Projects
None yet
Development

No branches or pull requests

8 participants