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

v5 #751

Merged
merged 133 commits into from
Jun 12, 2020
Merged

v5 #751

merged 133 commits into from
Jun 12, 2020

Conversation

mjackson
Copy link
Member

@mjackson mjackson commented Nov 5, 2019

This PR is for version 5 which is a complete rewrite of version 4 that keeps most of its features while trimming a few that we don't really need. I'll keep this PR open and cut beta releases (npm tag next) until we're ready to release v5, at which point I'll merge to master.

New features:

  • Remove legacy browser support (pre pushState)
  • Add state to hash history
  • Use custom window when creating history objects
  • Better history.block API w/ tx.retry for retrying transitions
  • Fix location.pathname encoding issues
  • About 50% smaller
  • No dependencies

Removed features:

  • Removes basename support
  • Removes relative pathname support in hash + memory histories
  • Removes getUserConfirmation
  • Removes keyLength
  • Removes hashType

Fixes #624
Fixes #704
Fixes #723
Fixes #726

New features:
- Remove legacy browser support (pre pushState)
- Add state to hash history
- Use custom window when creating history objects
- Better history.block API (wip)
- Fix location.pathname encoding issues
- About 50% smaller
- No dependencies

Removed features:
- Removes basename support
- Removes getUserConfirmation
- Removes keyLength
- Removes hashType
- Removes relative pathname support in hash + memory histories

Still TBD:
- Missing pathname support in push/replace

Fixes #624
Fixes #704
Fixes #723
Fixes #726
Also, wait to call blockers on POP transitions until we have already
returned to the URL we just left.
@mjackson
Copy link
Member Author

mjackson commented Nov 6, 2019

These test sequences are failing in IE 11 and Edge:

I'm thinking these tests are just bad tests and probably need to go. We should just provide the same pathname, search, and hash we get from the browser on our location objects and avoid decodeURI completely (same approach @OliverJAsh took in 930f0f7). I believe this will fix the long-standing issue in #505 (and #745).

Any objections? /cc @timdorr @pshrmn

package.json Outdated
"tiny-invariant": "^1.0.2",
"tiny-warning": "^1.0.0",
"value-equal": "^1.0.1"
"loose-envify": "^1.2.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can probably drop this. It's for Browserify users for the process.env stuff. Honestly, that doesn't belong in packages; it's a bundler problem. I'm not even sure why including it in the packages themselves fixes it. But if it needs to get fixed, that should happen on Browserify's end, not on literally every package in npm that uses process.env.

@timdorr
Copy link
Member

timdorr commented Nov 6, 2019

Ugh, I am really not a fan of the flat source file. Flat bundles? Yes! Flat source? Not so much...

It makes it harder to navigate through a nested set of functions when they're not broken out into individual files. With files, I can retain the scroll position of each one and just switch between tabs to navigate back and forth, or jump around.

I do appreciate the drastically smaller code that you have to work with, so a single file becomes more practical (it would be untenable under v4), but I don't feel like it's reached that particular cutoff.

@mjackson
Copy link
Member Author

mjackson commented Nov 6, 2019

Do you mean v6? v5 is taken by 5.0.0

RR is currently at v5, but history is still at v4. This next major will be history v5.

I am really not a fan of the flat source file.

I've enjoyed working in a single file for this work, mostly because of all the code duplication between the implementations. Having them side by side makes it easy to compare them. But I'm not married to it. Also, I seem to remember webpack having a hard time doing tree-shaking when we had more than one module. Maybe this will make it easier for them.

Anyway, I was mostly asking for feedback about the encoding issues. I think those tests are bad and hoping we can just remove them and be done w/ encoding issues once and for all!

@pshrmn
Copy link
Contributor

pshrmn commented Nov 7, 2019

I'll take a closer look this evening, but I agree about history not touching encoding.

@timdorr
Copy link
Member

timdorr commented Nov 7, 2019

Sorry, I deleted that comment about the version number because I thought this was the Router repo :P

Webpack only tree shakes along separate module files, IIRC. I've completely abandoned it for Rollup or Parcel now anyways, so that may be out of date info. Regardless, we're spitting out flat bundles anyways, so the size reduction stuff is a moot point. Just use a proper bundler like Rollup and don't worry about it.

Anyways, I get what you're saying about code duplication reduction, but I would argue that's easier to see between multiple files. You can squint your eyes a bit, switch back and forth between some tabs, and literally see the abstraction visually. That's harder to do in a single file, IMHO.

I will say this is nitpicky stuff (on my end), and I don't really have other comments about the more functional changes. It seems all pretty solid. And it's a good chance to knock out some long-standing bugs. 👍

@mjackson
Copy link
Member Author

mjackson commented Nov 8, 2019

Just adding a note that I'd like to address #614 with the v5 release. Seems like setting forceRefresh at history object creation time isn't the best time to do it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants