fix(gatsby): don't remount matchPath pages #10261
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pieh
commented
Dec 3, 2018
| @@ -20,7 +20,7 @@ class PageRenderer extends React.Component { | |||
| replacementElement || | |||
| createElement(this.props.pageResources.component, { | |||
| ...props, | |||
| key: this.props.location.pathname, | |||
| key: this.props.pageResources.page.path, | |||
pieh
commented
Dec 3, 2018
| .waitForAPI(`onRouteUpdate`) | ||
|
|
||
| cy.window().then(win => { | ||
| cy.wrap(win.___PageComponentLifecycleCallsLog).snapshot() |
There was a problem hiding this comment.
using snapshot testing, lifecycle calls log is quite verbose and might be hard to read - it might make sense to not use snapshots here and use more programmatic assertions
There was a problem hiding this comment.
One way to test this would be count renders in local state and add that to the DOM and assert against it
There was a problem hiding this comment.
It was kind of already doing that, but I did some refactoring to be more readable, so now I have:
// we expect just 1 `componentDidMount` call, when navigating inside matchPath
cy.lifecycleCallCount(`componentDidMount`).should(`equal`, 1)
cy.lifecycleCallCount(`render`).should(`equal`, 3)for story like this:
- load
/client-only-paths - navigate to
/client-only-paths/profile - navigate to
/client-only-paths/dashboard
DSchau
approved these changes
Dec 5, 2018
| <Link to="/long-page/" data-testid="long-page"> | ||
| To long page | ||
| </Link> | ||
| <pre data-testid="dom-marker">{pageContext.DOMMarker || `index`}</pre> |
There was a problem hiding this comment.
This is pretty slick. Nice work!
|
Thanks for the turnaround on this one! Was pulling my hair out on the undefined.fm's upcoming job board on this. You all rock! |
m-allanson
added a commit
to Bouncey/gatsby
that referenced
this pull request
Dec 7, 2018
* master: (1595 commits) chore: update link for react-gatsby-firebase-authentication (gatsbyjs#10314) fix(www): Awesome Gatsby sidebar link (gatsbyjs#10313) Add thijs koerselman to creators list (gatsbyjs#10303) chore(release): Publish fix(gatsby-plugin-emotion): allow for React.Fragment shorthand syntax (gatsbyjs#10291) feat(www): Update starter cards (gatsbyjs#10258) Update index.md (gatsbyjs#10307) Update index.md (gatsbyjs#10306) Add thijs koerselman portfolio to sites list (gatsbyjs#10304) chore(release): Publish fix(gatsby): don't remount matchPath pages (gatsbyjs#10261) chore(release): Publish feat(gatsby-source-contentful): enable RichText for all users (gatsbyjs#10301) docs(tutorial): update 404 screenshot (gatsbyjs#10295) feat(gatsby-plugin-typescript): allow specifying babel preset options (gatsbyjs#10248) docs(gatsby-plugin-sass): fix typo in docs (gatsbyjs#10292) fix(ci): actually only run tests on non-docs changes (gatsbyjs#10287) fix(blog): rename for correct date in link (gatsbyjs#10290) Adds a more descriptive link purpose (gatsbyjs#9266) fix(www): comment out down showcase site ...
gpetrioli
pushed a commit
to gpetrioli/gatsby
that referenced
this pull request
Jan 22, 2019
* fix: don't remount page component when navigating inside matchPath * test: add tests to ensure remounting behaviour stays consistent and avoid future regressions * switch from snapshot testing to more descriptive assert * abstract lifecycleCallCount, so tests are more readable
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Actual fix is one line change - most of this PR is cypress testing to catch any behaviour changes in future PRs
fixes #10227