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

go() query string repetition bug #93

Closed
drone1 opened this issue Dec 17, 2021 · 5 comments
Closed

go() query string repetition bug #93

drone1 opened this issue Dec 17, 2021 · 5 comments
Labels

Comments

@drone1
Copy link

drone1 commented Dec 17, 2021

I'm having an issue:

  • Give an expressive description of what is went wrong

Before I realized I can just call FlowRouter.setQueryParam(), I was running code that repeatedly (in my case, each time I would click a button), results in the query string param 'test' being repeated on the URL:

const route = FlowRouter.current()
const qs = {...route.queryParams} // clone
qs.test = 1
FlowRouter.go(
    route.path,
    {...route.params},	// clone
    qs
)

Shouldn't this be like calling setQueryParam()? Instead it results in the current path etc but adds ?test=1?test=1?test=1... to the URL. I would expect /path?test=1

  • Version of flow-router-extra you're experiencing this issue

3.7.5

  • Version of Meteor you're experiencing this issue

2.0

  • Browser name and its version (Chrome, Firefox, Safari, etc.)?

Chrome Version 96.0.4664.110 (Official Build) (64-bit)

  • Platform name and its version (Win, Mac, Linux)?
    Windows 10 20H2
@dr-dimitru dr-dimitru added the bug label Jun 7, 2022
dr-dimitru added a commit that referenced this issue Jun 8, 2022
__Major Changes:__

- ⚠️ It is recommended to set `decodeQueryParamsOnce` to `true`, fixing #78, pr #92, thanks to @michaelcbrook
- ⚠️ By default use `{decodeQueryParamsOnce : true}` in tests
- ⚠️ Deprecated `staringatlights:inject-data`, in favor of `communitypackages:inject-data`
- ⚠️ Deprecated `staringatlights:fast-render`, in favor of `communitypackages:fast-render`

__Changes:__

- 👷‍♂️ Merged #92, thanks to @michaelcbrook and @Pitchlyapp, fix #78
- 👨‍💻 Fix #93, thanks to @drone1
- 🤝 Support and compatibility with `communitypackages:inject-data`
- 🤝 Support and compatibility with `communitypackages:fast-render`
- 🤝 Compatibility with `[email protected]`

__Other changes:__

- 📔 Overall documentation update and minor refinement
- 👨‍🔬 Update test-suite to cover #93
- 🧹 Overall codebase cleanup
- 👷‍♂️ Removed unnecessary "monkey patching" around `Blaze.remove()`, thanks to @jankapunkt
  - 🔗 Related: meteor/blaze#372
  - 🔗 Related: meteor/blaze#375

__Dependencies:__

- 📦 `[email protected]`, *was `v6.5.2`*
@zeearth
Copy link

zeearth commented Jun 16, 2022

@dr-dimitru hi, your commit generate a bug we have more than one parameter optional into pathDef.
Our pathdef : '/companies/:id?/:action?/'
When we call the path method : FlowRouter.path('companies', { id: this._id._str, action: mode })
the generated path is no more correct : "/companies/6135cb32d14df059605901fd?%2F%3Aaction="

in 3.7.5 of the package the generated path is like this : "/companies/6135cb32d14df059605901fd/edit"

thanks for you return

@dr-dimitru
Copy link
Member

Hello @zeearth thank you for reporting.

This case is part of the tests.

Can you post values of console.log({id: this._id._str, action: mode}) looking ant the generated path mode is null or empty string?

@zeearth
Copy link

zeearth commented Jun 17, 2022

Hello @dr-dimitru

I saw your test, but my parameters are optionnal. I'm thinking problems come from the split on the '?' router.js:226 I think the split is done for separate uri and query string.
this._id._str is a common mongodb id string "629f87f44ec0e563c4d5c458" mode is a string view or edit.

dr-dimitru added a commit that referenced this issue Jun 17, 2022
dr-dimitru added a commit that referenced this issue Jun 17, 2022
dr-dimitru added a commit that referenced this issue Jun 17, 2022
- 🐞 Fix: #93, thanks to @zeearth
- 📦 Update `[email protected]`, *was `v.6.10.3`*
@dr-dimitru
Copy link
Member

@zeearth seems like I finally caught this one, try running on updated v3.8.1. lmk

@zeearth
Copy link

zeearth commented Jun 27, 2022

Good for us @dr-dimitru.
Thanks for the quick buck fix and for all you work.

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

No branches or pull requests

3 participants