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

Remove redundant '?' from search in createLocation #750

Closed
wants to merge 1 commit into from

Conversation

vimkin
Copy link

@vimkin vimkin commented Oct 28, 2019

Here is a fix for an issue that is marked for 4.11 release in a following roadmap #689 and reported in the #701. It appears this issue is caused because of createLocation and not createHref as it is stated in the roadmap. The former was just copying it without modifying to the output.

More in depth:

  1. Link from react-router-dom calls navigate on click
navigate() {
  // e.g. 'to' equals to { pathname: "/the/path", search: `?` }
  const location = resolveToLocation(to, context.location);
  const method = replace ? history.replace : history.push;

  // location descriptor is passed unchanged to corresponding history method
  // { pathname: "/the/path", search: `?` }
  method(location);
}
  1. history.push or history.replace are called and here is some common code from theirs body
// creates location from location descriptor passed from Link component
// path equals to { pathname: "/the/path", search: `?` }
const location = createLocation(path, state, createKey(), history.location);

// ... afterwards it sets location to the history state
setState({ action, location });
  1. Since '?' wasn't cleaned anywhere it ended up in the next state and caused redundant refetches, rerenders etc.

@vimkin
Copy link
Author

vimkin commented Oct 28, 2019

Super strange fail of the job with TEST_ENV=source: Edge 17.17134.0 (Windows 10.0.0) a browser history push state calls change listeners with the new location FAILED. May it be flaky CI bug because everything else is perfectly running without errors?

@trymbill
Copy link

trymbill commented Feb 3, 2021

So ... this being merged anytime soon?

@chaance chaance deleted the branch remix-run:master August 14, 2021 18:04
@chaance chaance closed this Aug 14, 2021
@chaance
Copy link
Contributor

chaance commented Aug 14, 2021

This was unintentionally closed because I deleted the master branch after renaming to main, very sorry about that! Feel free to open a new PR, but know I've bookmarked this for review either way.

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

Successfully merging this pull request may close these issues.

None yet

3 participants