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

Manage history state length exploding if clicking on a link with with the current route location #2616

Merged
merged 4 commits into from
May 25, 2021

Conversation

KrisCoulson
Copy link
Contributor

@KrisCoulson KrisCoulson commented May 24, 2021

Closes #1584

Relates #2567

Comment on lines 12 to 42
it('does add to the history if they are already at that location', () => {
expect(global.history.length).toEqual(2)
navigate('/test-2')
expect(global.history.length).toEqual(3)
navigate('/test-3')
expect(global.history.length).toEqual(4)
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thoughts on this test. I was having a hard time figuring out how to clear the global history. Open to changes if necessary

Copy link
Contributor

Choose a reason for hiding this comment

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

I know history is readonly in the browser, but I wonder if that's true for jest.

I don't think it's a huge problem, since I think history is reset for every test file.

Copy link
Contributor Author

@KrisCoulson KrisCoulson May 24, 2021

Choose a reason for hiding this comment

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

Yeah I searched around for a bit couldn't find a way. Tried something like
global.history = new History()
but typescript yells that it is an illegal constructor.

Did a bit of googling but most of the answers out there were for react-router and they were using a mock of their custom history obj.

Dunno if there is a way to reset the environment between the tests And I believe it does reset the history after every test file though

Copy link
Contributor

Choose a reason for hiding this comment

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

🆒 🆒 🆒 , I actually believe I read the same, but I couldn't find it in the docs. Maybe the best thing to do is leave a comment that we believe the history is reset in each test file, but couldn't find docs for it.

Copy link
Contributor

@peterp peterp left a comment

Choose a reason for hiding this comment

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

Thanks so much for this! I've got a question about comparing the other mutable "location" things, but overall this looks good to me.

Much love for adding tests!

Comment on lines 12 to 42
it('does add to the history if they are already at that location', () => {
expect(global.history.length).toEqual(2)
navigate('/test-2')
expect(global.history.length).toEqual(3)
navigate('/test-3')
expect(global.history.length).toEqual(4)
})
Copy link
Contributor

Choose a reason for hiding this comment

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

I know history is readonly in the browser, but I wonder if that's true for jest.

I don't think it's a huge problem, since I think history is reset for every test file.

packages/router/src/history.tsx Outdated Show resolved Hide resolved
@jtoar jtoar added this to the next-release-candidate milestone May 24, 2021
@KrisCoulson
Copy link
Contributor Author

@peterp I updated the tests and the navigate function to handle hash, search, and pathname. Don't love that the tests could be brittle but still unsure if there is a way to reset the history object for each test.

Copy link
Contributor

@peterp peterp left a comment

Choose a reason for hiding this comment

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

Awesome stuff!

@thedavidprice thedavidprice merged commit 85af627 into redwoodjs:main May 25, 2021
@thedavidprice
Copy link
Contributor

Huge thanks @KrisCoulson 🚀

Tobbe pushed a commit to dac09/redwood that referenced this pull request May 28, 2021
… the current route location (redwoodjs#2616)

* redwoodjs#1584 manage history state length

* handle pathname, search, and hash

* remove console.log

Co-authored-by: David Price <thedavid@thedavidprice.com>
dac09 added a commit to dac09/redwood that referenced this pull request Jun 2, 2021
…ter-tests

* 'main' of github.com:redwoodjs/redwood:
  downgrade jest-watch-typeahead 0.6.3 (redwoodjs#2699)
  Exclude yarn packages cache. (redwoodjs#2697)
  build(deps): bump ts-morph from 10.1.0 to 11.0.0 (redwoodjs#2656)
  build(deps): bump core-js from 3.12.1 to 3.13.1 (redwoodjs#2680)
  bump react types and eslint packages patch version (redwoodjs#2695)
  build(deps): bump esbuild from 0.12.1 to 0.12.5 (redwoodjs#2654)
  upgrade misc packages with patch (redwoodjs#2694)
  Fix lerna canary publishing; use default --canary versioning with `git describe` (redwoodjs#2693)
  Upgrade axios due to security alert. (redwoodjs#2688)
  add esbuild config to CLI build and dev (redwoodjs#2564)
  set up yarn offline cache (redwoodjs#2669)
  Create file watch for type def generation (redwoodjs#2614)
  Pin package dependencies, remove CRWA template yarn.lock, set up Yarn offline cache (redwoodjs#2637)
  Fix serve tests (redwoodjs#2668)
  Add script to create a functional test project using the latest CRWA template (redwoodjs#2324)
  build(deps): bump @typescript-eslint/eslint-plugin from 4.24.0 to 4.25.0 (redwoodjs#2627)
  Manage history state length exploding if clicking on a link with with the current route location (redwoodjs#2616)
  [forms] Fix number validation msg (redwoodjs#2552)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/confirmed We have confirmed this is a bug topic/router-&-navigation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The <Link> component increases the length of window.history when path doesn't change
4 participants