Deprecate pushState and replaceState for new push and replace

[taion]

No description provided.

[taion]

I'm like ~65% on this.

We lose a bit of conciseness by introducing this concept of a "location spec" object, but I think it's worthwhile to not entirely merge this concept into that of a "location", because of the omission of the action and key fields.

I'd be totally fine with merging this into the concept of a location if people feel that would make more sense, though.

[mjackson]

👍 I think this is the way to go.

[taion]

This builds on top of the updated CHANGES format per #167, because I wanted to call out new features separately from bug fixes.

[taion]

Added a WIP with the deprecations. Still need to change the tests - thinking of putting together a codemod for that.

[mjackson]

Do we really need to further sub-divide this doc? I was hoping to keep each release small enough that we wouldn't have to.

[taion]

What do you think about labelling it as something like:

• Feature: Accept objects in history.push and history.replace ([Accept objects in push and replace #141])
• Deprecation: Deprecate history.pushState and history.replaceState in favor of passing objects to history.push and history.replace ([Deprecate pushState and replaceState for new push and replace #168])
• Bugfix: Disable browser history on Chrome iOS ([Disable browser history on Chrome iOS #146])
• Bugfix: Do not convert same-path PUSH to REPLACE if the hash has changed ([Consider the full path in changing PUSH to REPLACE #167])

I just want an easy way for people looking at this to tell at a glance what has changed in which way - especially new features and deprecations, which might require some action on their part.

[mjackson]

Looking good, @taion 👍 Just let me know when you're ready to merge.

[taion]

Probably not today unless it's later :

I was hoping to use JSCodeShift to update the tests, but I have to learn how to use JSCodeShift first.

[taion]

I mean, not because there are so many tests that I can't update them by hand, more that I think JSCodeShift is super cool, and that it'd be nice to offer a codemod to our users.

[taion]

@timdorr

Continuing from remix-run/react-router#2646 (comment), the "location descriptor" or "location spec" or whatever is slightly different enough that we'd avoid some confusion by calling it something else. There's no action or key attached to it, so it's not exactly the same.

One example of the difference is that when using queries, your "location descriptor" might look like:

{
  pathname: '/foo',
  query: { the: 'query' },
}

But the corresponding "location" is more like:

{
  pathname: '/foo',
  query: { the: 'query' },
  search: '?the=query',
  action: 'POP',
  key: 'asdf12',
}

It's really a shorthand object that is enough to describe or specify a location, but is not a location itself.

[taion]

Got the codemod running: https://gist.github.com/taion/0b752445e86304f5f5c2

Doing final cleanups now, will update PR shortly.

What do we want to do with the transform? Ideally we'd ship it to users somehow.

[taion]

PR updated. This is ready to go from my end, assuming we don't want to ship the transform with this package.

I'm really impressed with JSCodeShift. All the test updates on the 2nd commit were with JSCodeShift, except:

• I manually backed out the change in BrowserHistory-test.js (I was too lazy to specifically omit window.history.pushState calls from the transform)
• I moved the PUSH -> REPLACE logic from describePushState.js to describePush.js
• The transform gave me double quotes instead of single quotes on string literals and I had to run npm run lint -- --fix to clean those up

[mjackson]

Thanks @taion. I'll take a look.
On Sat, Dec 5, 2015 at 4:19 PM Jimmy Jia notifications@github.com wrote:

PR updated. This is ready to go from my end, assuming we don't want to
ship the transform with this package.

I'm really impressed with JSCodeShift. All the test updates on the 2nd
commit were with JSCodeShift, except:

• I manually backed out the change in BrowserHistory-test.js (I was
  too lazy to specifically omit window.history.pushState calls from the
  transform)
• I moved the PUSH -> REPLACE logic from describePushState.js to
  describePush.js
• The transform gave me double quotes instead of single quotes on
  string literals and I had to run npm run lint -- --fix to clean those
  up

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

[timdorr]

You could drop it in the scripts folder. That's probably the best place for it. It's an optional thing anyways.

[taion]

Sorry, had to manually update a few more tests. There were a few meant to intentionally cover pushState and replaceState that I restored to their old forms, and I had to update a few test descriptions as well.

[taion]

@timdorr It's not a script per se, though - you can only run it through jscodeshift, so it's not especially helpful on its own.

[timdorr]

Yeah, but you would run it once for automating the upgrade to this API version. That sounds script-y to me 😄

[taion]

Oh, I missed the "single quote" recast option. Well, ESLint fixed it for me anyway.

@timdorr I was thinking of using transforms/ instead, though - it's what the upstream codemod packages do: https://github.com/reactjs/react-codemod, https://github.com/cpojer/js-codemod/. They're set up as separate packages there, though.

[taion]

Assuming we go forward with this PR as-is or in slightly modified form, I'm going to set up a rackt-codemod repo under this org, and distribute the codemod there.

[timdorr]

I was just going to suggest this. If you don't mind, could you create separate folders for each repo that has codemods (just history for now)?

[taion]

👍 Will do.

[mjackson]

@taion Looks like you're ready to merge here?

[mjackson]

👍 for this name. I was actually thinking about using this name too :)

[taion]

Yes, this is good to go from my end. Anything else I need to fix before we merge?

[timdorr]

@taion Needs a rebase on master now.

[taion]

Yup, done! Thanks.

[mjackson]

Thanks, @taion. This turned out to be quite a bit of work!

[taion]

It was fun, though, and the long-term API cleanup is going to be really sweet. Just need to fix createPath and createHref and deprecate createLocation.

[taion]

Oh crud, the warnings for state without queryKey aren't there on push and replace for createHashHistory. Bah.