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

react-router should re-export necessary functions from the history project #2211

Closed
jlongster opened this issue Oct 8, 2015 · 45 comments
Closed

Comments

@jlongster
Copy link
Contributor

history is a great project, but if I want to use react-router, I find it surprising that I have to install it as well as a top-level dependency, when it's really just a dependency for react-router. I could understand it if I'm doing something really low-level and internal, but if all I want to do is use a browserHistory instead of hashHistory (which is probably one of the most common use cases) I have to suddenly deal with this other library.

I suggest that react-router should just re-export the few things necessary from history that users need, like createBrowserHistory.

How do you make sure that I have the right history version installed? It's now a peer dependency, essentially.

For example, I upgraded to [email protected] and upgrade history to the latest version. But then I wanted to downgrade (for no reason of react-router, I was just debugging something). I removed react-router and re-installed 1.0.0-rc2. After I did this, I had two versions of history:

cq0itizwoaawgpg

The top-level one is the more up-to-date one, and npm (rightly) installed the old one for react-router. As expected I got an obscure error when trying to run it.

Hiding the history project within react-router would avoid users having to manage two separate projects that are actually dependent.

@mrtnbroder
Copy link

I strongly agree with this.

@taion
Copy link
Contributor

taion commented Oct 9, 2015

Would it be awful to make it an actual peer dependency? React Router seems to me like a plugin to History, in the same way that various components are plugins to React.

@jlongster
Copy link
Contributor Author

@taion peer deps are deprecated and going to be removed at some point, if they aren't already. They are super confusing for similar reasons I stated above (even with npm trying to help).

@mjackson
Copy link
Member

mjackson commented Oct 9, 2015

Thanks for starting this discussion, @jlongster. I agree the current situation isn't ideal.

Maybe the easiest thing to do is

export * from 'history'

in our top-level exports. The way I see it, react-router is essentially an extension of the history package. A "router" is basically history + routes. So re-exporting everything from history gives us:

  • the ability to "hide" history inside react-router so users don't have to explicitly install it
  • the ability for us to pin react-router to a specific history version
  • all of the tools that history gives us. this is important so we don't have to create our own abstraction on top of the history API (which <Router> currently is, but hopefully we can move away from that)

The main downside to this is that our UMD build is going to be a lot bigger than it needs to be. I was originally thinking that, in order to make builds as small as possible we could recommend to people that they reach into the lib directory and only import the pieces of history/react-router that they need. I'd like to preserve the ability to do this for people who need smaller builds.

Thoughts?

@taion
Copy link
Contributor

taion commented Oct 9, 2015

@jlongster

Where did you see that? It's true that they don't get auto-installed any more on NPM3, so the install instructions would have to be npm i -S react-router history if History were made a peer dependency, but I haven't seen anything official saying that they're going away.

@mjackson

I think this is a little strange in the sense that, at least recently, History has been releasing much faster. Suppose someone adds createFancyNewHistory to History - how would you manage that if history were a dependency of react-router? It seems like in that case users would have to separately install history anyway.

Though for the UMD build, would it not be possible to specify history per se as an external, but use a history/lib import for the parts we want to bundle?

@SpainTrain
Copy link

AFAIK peer deps is an on going discussion - npm/npm#5080 (that may never end X-D)

I'm not sure this is a valid use case for peer deps, though. The history dep is pinned to a specific version in package.json and in @jlongster's example, installing a version of history two patch versions away from react-router's version causes errors. This is a pretty tight coupling and probably not appropriate for peer deps.

Regarding bundle sizes, would it be reasonable to start by just covering 80% of use cases by only providing createBrowser/Memory/HashHistory? The re-exports could even be in separate files in modules so that they can be imported separately.

history may be releasing quickly, but are new history implementations being created that regularly? If not, keeping up-to-date doesn't seem intractable. Also, if having a different version installed at the top-level break things anyway (as it did for @jlongster and myself) then one doesn't have the option to use the newest history anyhow.

A related question: why is the history dep pinned?

@gaearon
Copy link
Contributor

gaearon commented Oct 9, 2015

@taion peer deps are deprecated and going to be removed at some point, if they aren't already.

This is misinformation that people keep spreading because peerDeps were bad in NPM 2 :-)

NPM 3 doesn't deprecate peer dependencies. It just changes their behavior to be warnings instead of hard errors, and that they don't cause automatic installs. But they stay. The point is that the developer is aware the peer dep is unmet, but isn't forced to correct the mistake, and has to make an explicit choice as to which (if any) version to depend on. That's it.

From npm/npm#6565 (comment):

To be clear, peerDependencies aren't going away.

I made a GIF so you can crosspost it to other relevant threads in the future:

bart-simpson-generator

@gaearon
Copy link
Contributor

gaearon commented Oct 9, 2015

Personally I think that:

  • history should be a peer of react-router, with only major version specified (caret range)
  • history shouldn't break API :-)
  • we should recommend people to use [email protected] for saner handling of peerDeps

This way router can depend on history API, but user may update history for patch fixes and new features.

@taion
Copy link
Contributor

taion commented Oct 9, 2015

I agree entirely with @gaearon's:

history should be a peer dependency of react-router, with only major version specified (caret range)

I think this is the most correct way to set things up. There's a small cost in forcing users on NPM3 to npm i history react-router, but I don't think that's material.

@SpainTrain
Copy link

Hah, yes, following semver would certainly make this a reasonable use of peer deps. 😂 Agree that it's the best possible outcome. Doable in practice?

@taion
Copy link
Contributor

taion commented Oct 9, 2015

Is History not currently follow semver?

@mjackson
Copy link
Member

mjackson commented Oct 9, 2015

Is History not currently follow semver?

Uh, I thought I was.

@jlongster @SpainTrain Is there some way I violated semver between history 1.12.1 and 1.12.3?

Sounds like this is the perfect use case for a peer dep.

@taion
Copy link
Contributor

taion commented Oct 9, 2015

Wow, English language fail. That was directed at @SpainTrain. I meant to assert that as far as I could tell, History was following semver just fine.

You know who would definitely disagree with this, though? @ljharb. Because unlike with React, it's also the case that nothing would actually break if there were two versions of history installed.

@SpainTrain
Copy link

Apologies, I didn't mean to imply there wasn't good faith effort to follow semver, but whether it was effective in practice. That's why I asked why the history dep was pinned, rather than a semver range. Also thought that was the implication of the comment "history shouldn't break API :-)"

The issue I encountered would have been preventable with an up-to-date peer dep, so sharing the error I got is not useful. @jlongster could you share the details of the obscure error you mentioned? The diff looks fine, so maybe it's an edge case?

@mjackson
Copy link
Member

mjackson commented Oct 9, 2015

Ah, no need to apologize. It wouldn't be the first time I've been a semver bad guy :)

@taion
Copy link
Contributor

taion commented Oct 9, 2015

Oh, that is weird - why is the dependency pinned?

@mjackson
Copy link
Member

mjackson commented Oct 9, 2015

Because I'm scared I'll break my own library? I dunno.

@taion
Copy link
Contributor

taion commented Oct 10, 2015

RE: peer deps, @ljharb brought up an interesting point -

If you make [email protected] a peer dependency of react-router, then you break the case where (suppose one day a [email protected] comes out) a user wants to separately use [email protected], but still wants to use react-router, but not pass in a hypothetical history object from the history v2 library.

I'm not convinced that this use case really matters, but it's an interesting point. ¯_(ツ)_/¯

@jlongster
Copy link
Contributor Author

Sorry to spread misinformation about peer deps.

I still don't understand why history just isn't a normal dependency, when it clearly is... a dependency. If, for some reason, I want to directly use history it's not hard for me to install it manually like we're forced to do now. But I don't think users of react-router should have to think about it (maybe if using more esoteric features, I dunno).

@jlongster
Copy link
Contributor Author

The only stated benefit so far is that it's easy to keep UMD builds lean. I'm sure we could figure out a solution to that, and it seems like a very minor benefit for more hassle.

@taion
Copy link
Contributor

taion commented Oct 11, 2015

It is a dependency.

The problem is that just re-exporting a library that potentially changes under you seems prone to inconsistent behavior.

As long as history doesn't change its API, it seems like it'd be better to encourage the user to specify his or her version of history explicitly; compared to the re-export which gives you some mystery version of history that the user can't directly control.

@jlongster
Copy link
Contributor Author

It can't change under you. Any changes will dictate a new react-router version as well.

If you are calling history methods directly, you can simply install it yourself as we do now and hope that it works with the current version of react-router.

I may be viewing this differently. I'm honestly a naive outside user who just came here to use this library. To me I just want to install one project, read docs on one project, etc, and be done.

@taion
Copy link
Contributor

taion commented Oct 11, 2015

That's only the case if React Router pins the dependency on History to a specific version, no? If React Router specifies a version range, then all bets as to the actual installed version of History are off.

If you go with pinning the History dependency, then you end up having to release a new version of React Router every time you update History. At that point, why not just merge History back into React Router?

That last question isn't rhetorical, BTW. From the PoV of making things as easy as possible for users of React Router, it'd probably be easiest if History were just part of the library. Otherwise, I think the best compromise would be to make History a peer dependency, to allow separate versioning, without raising the question of "what about this instance of history that react-router depends on".

@ljharb
Copy link

ljharb commented Oct 11, 2015

fwiw, keeping builds clean is the job of the consumer, not the publisher. browserify/webpack/et al all make this easy, and there's zero reason any publishing module be concerned about build size, or UMD at all, in 2015.

@timdorr
Copy link
Member

timdorr commented Oct 11, 2015

So, I had proposed exporting our own wrapper for createMemoryHistory this week: #2175 (comment)

What if we did that and also exported createBrowserHistory and createHashHistory? They're the most common use cases for exporting history's API.

@taion
Copy link
Contributor

taion commented Oct 11, 2015

Thinking through this a bit, then, I think the way we'd handle re-exports from a semver POV would be to specify the version on history like 1.12.x or ~1.12.4, and bump the minor version on react-router every time we bump the minor version dependency on history.

Would that be acceptable? I still think the peer dependency solution is a bit cleaner, though.

@knowbody
Copy link
Contributor

I would go with the peer dependency solution. In my opinion main reasons is that it does not make sense to bump up the versions of both history and react-router. Especially that React Router can work without the History module.

Thinking a bit forward what if we add scroll module (for managing the scroll behaviour) in the future, or any other module? I think it won't be a good practice to change the version of the react-router every time any of those change.

@ljharb
Copy link

ljharb commented Oct 11, 2015

I think it won't be a good practice to change the version of the react-router every time any of those change.

Of course it would. It's good practice to change the version every time anything changes, big or small, as frequently as is possible. Version numbers aren't sacred, and the appropriate major/minor/patch should be bumped aggressively.

@jlongster
Copy link
Contributor Author

Especially that React Router can work without the History module.

Is that really true? I mean, do we really ever expect people to use it without the history module, and re-implement everything that history does? This seems like a case of over-modularization.

@knowbody
Copy link
Contributor

@ljharb But those won't be the changes to react-router itself, but to the separate module(s).

Don't get me wrong, obviously I face the same issues now as you are facing and it is a little pain to install history and react-router as that's the way most ppl use it.
I just would go with the peer dependency solution.

@ljharb
Copy link

ljharb commented Oct 11, 2015

@knowbody which should cause corresponding dep changes in react-router, and a corresponding version bump.

Especially that React Router can work without the History module.

Perhaps this is part of the problem? If it only worked without it, there'd be no issue - if you gave it something that wasn't a history object, it'd simply throw, and "optionalDependencies" or "peerDependencies" might help warn people at install time. If it only worked with it, it's a strict dependency. By doing both, this question comes up. (In addition, history may want to provide a simple way of checking that a given instance was from the correct version, at runtime, so that consumers don't need to rely on questionable install-time checks)

@taion
Copy link
Contributor

taion commented Oct 11, 2015

If you re-export pieces of History in React Router, semver requires that minor bumps to History lead to minor bumps in React Router, because otherwise you'd be e.g. adding a new feature to ReactRouter.createBrowserHistory (via History.createBrowserHistory) without making a minor version bump.

I don't object to making these bumps per se, but it does mean that you'd need to release React Router more often than you otherwise would, i.e. whenever History has a minor version bump.

If we don't expect people to use React Router without also explicitly using a (compatible) version of History, then it seems like a peer dep would be the right way to go. In other words, if it weren't for the default hash history, I'd think that a peer dependency just would be the right way to go.

@ljharb
Copy link

ljharb commented Oct 11, 2015

It's npm. Releasing a new version should be cheap and easy and frequent, and not something to be afraid of.

@knowbody
Copy link
Contributor

I think @ljharb got the point here. But I'm still not 100% sure that this is the best solution. I can just see ppl sending issues "hey I'm trying to do this in 1.0.27 and it doesn't work" and us wondering what has changed since.

Also what if someone wants to use their own implementation of history?

@mjackson
Copy link
Member

React Router can work without the History module

We're not very far from completely decoupling the router from history, but we're not quite there yet. Places where we still depend on history:

So, while I'm hopeful that at some point in the future we can completely decouple the router from the history package, we still actually do depend on it for the time being.

the appropriate major/minor/patch should be bumped aggressively

Agreed. I'm not scared at all of releasing new versions. Let's release as frequently as possible.

keeping builds clean is the job of the consumer, not the publisher

Agreed. I'm not terribly concerned about our UMD build size. Just said that it's a downside of re-exporting everything from history. Not a huge deal.

For now, this seems like the canonical use case for a peer dependency.

@mjackson
Copy link
Member

I just want to install one project, read docs on one project, etc, and be done

If we go the peer dep route, you'll have to install two projects. You ok w that @jlongster?

This seems like a case of over-modularization

There are a few reasons I extracted the history package out of the router. I realize that over-modularization is a real problem in large packages, but I don't think that's what's going on here.

Most importantly, history is a really useful library without react router. This isn't a case of creating one function per module. There are a myriad of routing experiments with redux, like routex that took advantage of the fact that history was a separate package to try something new. It's my hope that many other client-side routing libraries can take advantage of the capabilities of the history package as well.

Besides that, keeping history in a separate package helped us to focus in on solving several long-standing bugs and edge cases that easily get lost in discussions about higher-level concepts, like routing. If you check out the history issues, you'll see some interesting stuff that has nothing to do with routing.

@taion
Copy link
Contributor

taion commented Oct 11, 2015

@ljharb I dunno, NPM seems to think it's a pretty big deal, with how they've hardcoded the name "history" and all.

@mjackson How do you picture React Router working without History? Would it be some one-shot routing with actual hard page transitions and the browser itself doing the actual history management?

@jlongster
Copy link
Contributor Author

I just want to install one project, read docs on one project, etc, and be done

If we go the peer dep route, you'll have to install two projects. You ok w that @jlongster?

Well, the peer dep route was something I was hoping not to do for that reason. But I suppose it's better than the current situation, where history is installed behind-the-scenes but later when I want to do a very common task (use browserHistory) I need to retro-actively change how it's installed. It does seem better to do either/or.

A big part is advertisement. history isn't even mentioned in react-router's README, and if it's really meant to be more of an "addon" to history then I think the wording should be changed for the project. Right now it reads like a full-blown routing solution, so it's surprising to be thrown to history pretty quickly.

I think if my expectations were different I wouldn't have been as surprised, and now I understand it better but I think it would help to mention history more prominently.

Besides that, keeping history in a separate package helped us to focus in on solving several long-standing bugs and edge cases that easily get lost in discussions about higher-level concepts, like routing. If you check out the history issues, you'll see some interesting stuff that has nothing to do with routing.

Yeah, sorry for the strong statement, it's not really a case of over-modularization (there are obvious reasons why history is separate, it has a lot of non-react stuff), but I was talking more about over-worrying about react-router just re-exporting all of history. I understand the intentions of the projects better now.

If you're not going to make react-router just re-export everything and update versions every time history change (which I still don't see a problem with), I definitely think it'd be good to force history to be installed up-front as a peer dep.

@taion
Copy link
Contributor

taion commented Oct 11, 2015

You're not meaningfully changing how it's installed, though. Why does the built-in history even matter? It's an implementation detail of React Router.

@jlongster
Copy link
Contributor Author

You're not meaningfully changing how it's installed, though. Why does the built-in history even matter? It's an implementation detail of React Router.

Very confused, I don't really know what you mean. That it's an implementation detail is exactly how I feel about history and why it should be a normal dep.

But I don't really care that much. If it's a peer dep, that's ok, just make me install it up-front. Not going to spend a whole lot more time on this, I'll just follow whatever decisions the maintainers make.

@taion
Copy link
Contributor

taion commented Oct 11, 2015

What I mean is - whether or not the react-router package has a dependency on the history package should be irrelevant to you. It's just another dependency for NPM to manage.

But in some sense, how you manage history is orthogonal to how you do routing. Someone could make a my-awesome-history package that is API compatible with history, and it might be nice if it were possible to use that instead of using history. And the converse as well.

Is this really just more of a documentation/advertisement issue like you mention on #2211 (comment), where history isn't even mentioned in the README? Maybe it'd be better just to fix the README instead of enforcing history as a peer dep.

@jlongster
Copy link
Contributor Author

@taion Whether something is a normal dep or peer dep is a big difference, as seen in my original post. We do have to be aware of where things are installed, realistically, whether it's node_modules/history or node_modules/react-router/node_modules/history. Granted, I haven't upgraded to the latest npm yet and they may do things that make this better. But you still generally need to be aware of what's going on.

Updating the documentation would definitely be the biggest help, yeah. But I also think that we should go peer deps full force if that's what we really are trying to do.

@knowbody
Copy link
Contributor

I do agree with @jlongster on the documentation part. @jlongster we will update the docs and clarify what React Router and History are and what role they are playing.

@taion
Copy link
Contributor

taion commented Oct 11, 2015

@jlongster

Now I don't understand. In general you shouldn't have to be aware of the direct dependencies of the libraries you install at all. Certainly it matters whether or not history is a peer dependency of react-router, but to someone installing the package, I don't see how there would be any direct observable difference between history being a dependency and history not being a dependency.

Although looking back at your initial example, you should be aware that the error you saw was most likely because the 1.0.0-rc2 release was broken due to an NPM bug. That's a separate issue - in general you should not "[get] an obscure error" by doing that sort of thing.

@SpainTrain
Copy link

👍 @ peer deps

Aside: something like http://greenkeeper.io/ would have caught/fixed the inadvertently pinned dep

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

9 participants