Deprecate pushState/replaceState, use push/replace instead

[mjackson]

We should deprecate the pushState/replaceState APIs and just use push/replace instead with an optional 2nd state argument. This is the API that @th0r originally suggested in #118, but I didn't get it at that time (not sure why).

This will save everyone passing null as the state argument to pushState and replaceState all the time when they're not using location.state.

[th0r]

👍

[taion]

I'm 👎 on this. The intention of #141 is to move the API forward by making push and replace take an object when attempting to specify additional arguments. This lets us move forward by making the history API fully generic, and allows e.g. third-party named route support by e.g. adding a history enhancer that changes the push API to be history.push({name, params}), as well as support potential complex RN use cases that don't map well to URLs.

Going back to a privileged multi-arg form and not offering a full replacement is more complicated for users and doesn't give us as good an API.

[th0r]

To be clear, I'm just for the deprecation of the *State methods and for the ability to specify state argument in push/replace.
Will it be the prop on the first argument or the second optional argument doesn't matter to me.

[taion]

Yup - understood, and in a vacuum I'd consider history.push(path, state, query) to be a superior API to history.pushState(state, path, query).

However, both APIs are worse than history.push({path, state, query}), which:

• Gives us a cleaner way to omit parts of the location
• Lets us do history.push({...location, query: newQuery})
• Gives us a very clean way to move forward on Universally allow strings or objects for locations react-router#2186
• Buys a lot of flexibility for API-compatible React Native support
• Allows proper userspace named routes support

Thus IMO we shouldn't set up the history.push(path, state, query) and instead move directly to the object-based one from #141.

[ryanflorence]

I'd vote:

// the majority case
push('/some/where')

// the rest
push({ path: '/some/where', query: {}, state: {} })

[timdorr]

I'm also for interchangeable string and objects. The object form is basically Javascript's version of keyword args from Ruby or Python (when you destructure the object arg). And kwargs are the bomb dot com, so I'm always up for that.

Another big advantage is the args are no longer ordered, so if we add more, there is less breakage during upgrades. Makes API changes less hairy and keeps work in related projects (router, etc.) to a minimum.

I'm with @taion and @ryanflorence on this one.

[taion]

Great, if you like that API, it's coded up on #141.

[taion]

And I'd really appreciate feedback on implementation, specifically regarding the workaround in useQueries.

[taion]

Given the above, are we okay with deferring this until after we merge #141, and then transitioning directly to the object-based push and replace APIs instead?

If so, we might want to edit the OP to note the object-based API, rather than optional args.

[mjackson]

It's not important to me that state is a separate arg. That's an API detail.

The important part of this issue is that the whole idea of state should be optional without the user having to explicitly pass null as the first argument to pushState and replaceState, since it is largely redundant, and that there should instead be a way to include state in a call to push and replace, making pushState and replaceState themselves redundant. I actually like the object-based approach, but the OP still stands. pushState and replaceState should be deprecated once push and replace get the ability to handle state.

[taion]

That makes sense. I'm actually coming at this from a different angle - I care a lot about the API to push v pushState, because just accepting an object for push means that we can make this API fully generic. For example, instead of doing

history.push({path, query})

Something like a useNamedRoutes enhancer can change the signature to:

history.push({name, params, query})

The flexibility has the potential to be invaluable in building out RN support, because it gives us a lot of power in iterating on the specific API for update specs, without needing to change any API contracts between the router and history.

I agree that it's an API detail, but to me it's a very important one - by asserting that the argument is a flexible object, and not making any assumptions on the rest of the signature, we end up ripping out all the assumptions about how locations are constructed, which is a lot of flexibility at relatively low cost.

[taion]

What sort of timing are we thinking here w/r/t release schedule? Do we want to fully drop pushState and replaceState for 2.x?

[taion]

One more thought - if we drop the browser-compatible APIs, I think we end up breaking people like @natew who per #121 (comment) appears to just be using history as a polyfill for the HTML5 history API.

[timdorr]

If you're setting up a window.history.pushState polyfill, you you can just do that on your own as a wrapper to whatever API history exposes.

window.history.pushState = function(state, title, url) { 
  hashHistory.push({ path: url, state: state})
}

I've always felt a little funky about co-opting a public API, but not explicitly being a polyfill library.

[taion]

Sure - we can also do e.g. useHistoryPolyfill or something that provides pushState and replaceState.

[mjackson]

Do we want to fully drop pushState and replaceState for 2.x?

Yes. I don't want to ship 2.x with two different APIs for push/replace.

I've always felt a little funky about co-opting a public API, but not explicitly being a polyfill library.

Agreed. Using the names push/replace gets us away from that problem.

[mjackson]

We're not a polyfill. If someone wants to make a polyfill using history, they should be able to do it easily. But I don't think we should be concerned with it.

[taion]

Yeah, easy enough to add an enhancer. The pushState -> push thing would also be a great fit for a JSCodeShift codemod - too bad nobody actually knows how to write codemods, though ):

#141 already made e.g. pushState just a thin wrapper around push. Do you think we should immediately add deprecation flags for the next minor release, then?

[mjackson]

Yes, let's deprecate them in the next minor release.
On Wed, Dec 2, 2015 at 2:54 PM Jimmy Jia [email protected] wrote:

Yeah, easy enough to add an enhancer. The pushState -> push thing would
also be a great fit for a JSCodeShift codemod - too bad nobody actually
knows how to write codemods, though ):

#141 #141 already made e.g.
pushState just a thin wrapper around push. Do you think we should
immediately add deprecation flags for the next minor release, then?

—
Reply to this email directly or view it on GitHub
#162 (comment).

[taion]

I'll add the deprecations to #168, since they're touching on the same docs there anyway.

Let's also figure out how to cut RR 1.1.0, then, since I don't want to show deprecation warnings to all of our users who are using <Link>.

[mjackson]

Let's follow up in #168.

[ayaniv]

Hi @mjackson, I wonder what was the reason you've decided to deprecate these functions out of the sudden.