From 1dd39877b8565ff0ea9b3537c515b6e4800876dd Mon Sep 17 00:00:00 2001 From: Michal Piechowiak Date: Wed, 5 Dec 2018 16:47:11 +0100 Subject: [PATCH] fix(gatsby): don't remount matchPath pages (#10261) * 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 --- .eslintrc.json | 2 +- .../cypress/integration/lifecycle-methods.js | 41 +++++++++++++++++ .../cypress/support/commands.js | 10 +++++ .../cypress/support/index.js | 1 + .../production-runtime/gatsby-browser.js | 25 ++++++++--- e2e-tests/production-runtime/gatsby-node.js | 24 +++++++--- .../src/pages/client-only-paths.js | 34 ++++++++++++++ .../production-runtime/src/pages/index.js | 44 +++++++++++++------ .../production-runtime/src/pages/page-2.js | 4 +- .../src/utils/instrument-page.js | 33 ++++++++++++++ packages/gatsby/cache-dir/page-renderer.js | 2 +- 11 files changed, 191 insertions(+), 29 deletions(-) create mode 100644 e2e-tests/production-runtime/cypress/integration/lifecycle-methods.js create mode 100644 e2e-tests/production-runtime/cypress/support/commands.js create mode 100644 e2e-tests/production-runtime/src/pages/client-only-paths.js create mode 100644 e2e-tests/production-runtime/src/utils/instrument-page.js diff --git a/.eslintrc.json b/.eslintrc.json index a5d012e448afb..4efa02222c841 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -59,7 +59,7 @@ } }, { - "files": ["**/cypress/integration/**/*"], + "files": ["**/cypress/integration/**/*", "**/cypress/support/**/*"], "globals": { "cy": false, "Cypress": false diff --git a/e2e-tests/production-runtime/cypress/integration/lifecycle-methods.js b/e2e-tests/production-runtime/cypress/integration/lifecycle-methods.js new file mode 100644 index 0000000000000..774e9346b9065 --- /dev/null +++ b/e2e-tests/production-runtime/cypress/integration/lifecycle-methods.js @@ -0,0 +1,41 @@ +describe(`Production build tests`, () => { + it(`should remount when navigating to different template`, () => { + cy.visit(`/`).waitForAPI(`onRouteUpdate`) + + cy.getTestElement(`page2`) + .click() + .waitForAPI(`onRouteUpdate`) + + // we expect 2 `componentDidMount` calls - 1 for initial page and 1 for second page + cy.lifecycleCallCount(`componentDidMount`).should(`equal`, 2) + cy.lifecycleCallCount(`render`).should(`equal`, 2) + }) + + it(`should remount when navigating to different page using same template`, () => { + cy.visit(`/`).waitForAPI(`onRouteUpdate`) + + cy.getTestElement(`duplicated`) + .click() + .waitForAPI(`onRouteUpdate`) + + // we expect 2 `componentDidMount` calls - 1 for initial page and 1 for duplicated page + cy.lifecycleCallCount(`componentDidMount`).should(`equal`, 2) + cy.lifecycleCallCount(`render`).should(`equal`, 2) + }) + + it(`should NOT remount when navigating within client only paths`, () => { + cy.visit(`/client-only-paths`).waitForAPI(`onRouteUpdate`) + + cy.getTestElement(`/profile`) + .click() + .waitForAPI(`onRouteUpdate`) + + cy.getTestElement(`/dashboard`) + .click() + .waitForAPI(`onRouteUpdate`) + + // we expect just 1 `componentDidMount` call, when navigating inside matchPath + cy.lifecycleCallCount(`componentDidMount`).should(`equal`, 1) + cy.lifecycleCallCount(`render`).should(`equal`, 3) + }) +}) diff --git a/e2e-tests/production-runtime/cypress/support/commands.js b/e2e-tests/production-runtime/cypress/support/commands.js new file mode 100644 index 0000000000000..62bedced4b6be --- /dev/null +++ b/e2e-tests/production-runtime/cypress/support/commands.js @@ -0,0 +1,10 @@ +Cypress.Commands.add(`lifecycleCallCount`, action => + cy + .window() + .then( + win => + win.___PageComponentLifecycleCallsLog.filter( + entry => entry.action === action + ).length + ) +) diff --git a/e2e-tests/production-runtime/cypress/support/index.js b/e2e-tests/production-runtime/cypress/support/index.js index 819e86d80e162..e1570cea495ef 100644 --- a/e2e-tests/production-runtime/cypress/support/index.js +++ b/e2e-tests/production-runtime/cypress/support/index.js @@ -16,6 +16,7 @@ // Import commands.js using ES2015 syntax: // TODO: import as "cypress-gatsby" once this is published to NPM import '../../../../packages/cypress-gatsby' +import './commands' // Alternatively you can use CommonJS syntax: // require('./commands') diff --git a/e2e-tests/production-runtime/gatsby-browser.js b/e2e-tests/production-runtime/gatsby-browser.js index b1e5c316b7f94..f81cf6249d3e9 100644 --- a/e2e-tests/production-runtime/gatsby-browser.js +++ b/e2e-tests/production-runtime/gatsby-browser.js @@ -1,7 +1,20 @@ -/** - * Implement Gatsby's Browser APIs in this file. - * - * See: https://www.gatsbyjs.org/docs/browser-apis/ - */ +if (typeof window !== `undefined`) { + window.___PageComponentLifecycleCallsLog = [] +} -// You can delete this file if you're not using it +const addLogEntry = (action, location) => { + const idElement = document.querySelector(`[data-testid="dom-marker"]`) + window.___PageComponentLifecycleCallsLog.push({ + action, + pathname: location.pathname, + domContent: idElement ? idElement.innerText : null, + }) +} + +exports.onPreRouteUpdate = ({ location }) => { + addLogEntry(`onPreRouteUpdate`, location) +} + +exports.onRouteUpdate = ({ location }) => { + addLogEntry(`onRouteUpdate`, location) +} diff --git a/e2e-tests/production-runtime/gatsby-node.js b/e2e-tests/production-runtime/gatsby-node.js index 2f4266513eb66..1b7aa08513b2e 100644 --- a/e2e-tests/production-runtime/gatsby-node.js +++ b/e2e-tests/production-runtime/gatsby-node.js @@ -1,7 +1,17 @@ -/** - * Implement Gatsby's Node APIs in this file. - * - * See: https://www.gatsbyjs.org/docs/node-apis/ - */ - -// You can delete this file if you're not using it +exports.onCreatePage = ({ page, actions }) => { + if (page.path === `/client-only-paths/`) { + // create client-only-paths + page.matchPath = `/client-only-paths/*` + actions.createPage(page) + } else if (page.path === `/`) { + // use index page as template + // (mimics) + actions.createPage({ + ...page, + path: `/duplicated`, + context: { + DOMMarker: `duplicated`, + }, + }) + } +} diff --git a/e2e-tests/production-runtime/src/pages/client-only-paths.js b/e2e-tests/production-runtime/src/pages/client-only-paths.js new file mode 100644 index 0000000000000..1a54ddca88921 --- /dev/null +++ b/e2e-tests/production-runtime/src/pages/client-only-paths.js @@ -0,0 +1,34 @@ +import React from 'react' +import { Router } from '@reach/router' +import { Link } from 'gatsby' + +import Layout from '../components/layout' +import InstrumentPage from '../utils/instrument-page' + +const Page = props => ( + 
[client-only-path] {props.page}
+) + +const routes = [`/`, `/profile`, `/dashboard`] + +const basePath = `/client-only-paths` + +const ClientOnlyPathPage = props => ( + + + + + + + +) + +export default InstrumentPage(ClientOnlyPathPage) diff --git a/e2e-tests/production-runtime/src/pages/index.js b/e2e-tests/production-runtime/src/pages/index.js index f5bd91231948c..6986ba9ec71ce 100644 --- a/e2e-tests/production-runtime/src/pages/index.js +++ b/e2e-tests/production-runtime/src/pages/index.js @@ -2,22 +2,40 @@ import React from 'react' import { Link } from 'gatsby' import Layout from '../components/layout' +import InstrumentPage from '../utils/instrument-page' -const IndexPage = () => ( +const IndexPage = ({ pageContext }) => (

Hi people

-

Welcome to your new Gatsby site.

-

Now go build something great.

- - Go to page 2 - - - To non-existent page - - - To long page - + 
{pageContext.DOMMarker || `index`}
+ ) -export default IndexPage +export default InstrumentPage(IndexPage) diff --git a/e2e-tests/production-runtime/src/pages/page-2.js b/e2e-tests/production-runtime/src/pages/page-2.js index 4d3665d34cc85..5e936b25cb712 100644 --- a/e2e-tests/production-runtime/src/pages/page-2.js +++ b/e2e-tests/production-runtime/src/pages/page-2.js @@ -2,13 +2,15 @@ import React from 'react' import { Link } from 'gatsby' import Layout from '../components/layout' +import InstrumentPage from '../utils/instrument-page' const SecondPage = () => (

Hi from the second page

Welcome to page 2

+ 
page-2
Go back to the homepage ) -export default SecondPage +export default InstrumentPage(SecondPage) diff --git a/e2e-tests/production-runtime/src/utils/instrument-page.js b/e2e-tests/production-runtime/src/utils/instrument-page.js new file mode 100644 index 0000000000000..22543134e78cc --- /dev/null +++ b/e2e-tests/production-runtime/src/utils/instrument-page.js @@ -0,0 +1,33 @@ +import React from 'react' + +export default Page => + class extends React.Component { + addLogEntry(action) { + if (typeof window !== `undefined`) { + window.___PageComponentLifecycleCallsLog.push({ + action, + pageComponent: this.props.pageResources.page.componentChunkName, + locationPath: this.props.location.pathname, + pagePath: this.props.pageResources.page.path, + }) + } + } + + constructor(props) { + super(props) + this.addLogEntry(`constructor`) + } + + componentDidMount() { + this.addLogEntry(`componentDidMount`) + } + + componentWillUnmount() { + this.addLogEntry(`componentWillUnmount`) + } + + render() { + this.addLogEntry(`render`) + return + } + } diff --git a/packages/gatsby/cache-dir/page-renderer.js b/packages/gatsby/cache-dir/page-renderer.js index a8f500f66706c..403787fbbe1db 100644 --- a/packages/gatsby/cache-dir/page-renderer.js +++ b/packages/gatsby/cache-dir/page-renderer.js @@ -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, }) const wrappedPage = apiRunner(