FormidableLabs / redux-little-router

A tiny router for Redux that lets the URL do the talking.

MIT License

1.04k stars 114 forks source link

Fix `persistQuery` option for push, replace actions #226

Closed adamesque closed 6 years ago

adamesque commented 7 years ago

This PR adds awareness of persistQuery to the middleware, so that when we push/replace a new history state, the browser's URL includes the merged set of query params.

It also adds a new utility function, mergeQueries, which takes two query objects, merges them, and return a Location-compatible object with the merged queries and a merged search string.

Let me know what you think!

Fixes #224

codecov[bot] commented 7 years ago

Codecov Report

Merging #226 into master will increase coverage by 0.02%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #226      +/-   ##
==========================================
+ Coverage   98.87%   98.89%   +0.02%     
==========================================
  Files          19       20       +1     
  Lines         266      271       +5     
==========================================
+ Hits          263      268       +5     
  Misses          3        3

Impacted Files	Coverage Δ
src/util/merge-queries.js	`100% <100%> (ø)`
src/components/link.js	`100% <100%> (ø)`	:arrow_up:
src/reducer.js	`100% <100%> (ø)`	:arrow_up:
src/middleware.js	`95.83% <100%> (+0.59%)`	:arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 724b348...2637111. Read the comment docs.

PeterLeeHyfn commented 7 years ago

any updates on this PR?

adamesque commented 7 years ago

@PeterLeeHyfn I've been happily using my fork with this fix. This project appears to be dormant right now; seems like it's maintained by a single person & I don't begrudge them taking a break; open source takes an incredible amount of energy. If I had the bandwidth I'd offer to help maintain, but I can't really make that offer in good faith right now.

ryan-roemer commented 7 years ago

Hi @adamesque !

This one is a priority of ours and we're aiming to get back to the backlog soon. Biggest blocker is a small child cramping our main maintainer's style + we haven't been able to train up some other folks to better help maintain the repo.

It's on the radar, just not right now. Totally understand needing to get by with a fork in the meantime, and appreciate any patience y'all have for us as we try and get more support ❤️ going for this repo....

adamesque commented 6 years ago

Thanks for the review, @aweary! I pushed up a few commits to address your feedback.

markusenglund commented 6 years ago

It would be great if we could get this pull request merged and published soon. It's really needed.

ryan-roemer commented 6 years ago

@yogaboll -- I think we have just one more test request from @adamesque to go. If you want to pull this branch, create a test and just paste it here in a comment, we might able to go forward if @adamesque doesn't have time right now.

aweary commented 6 years ago

@ryan-roemer looks like that test was added (Github just didn't collapse my comment). @tptee if this looks good to you then it should be good to merge 👍

adamesque commented 6 years ago

tptee commented 6 years ago

Out in v14.2.2!