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

feat(develop): add explicit express handler for page-data requests #27882

Merged
merged 6 commits into from
Nov 12, 2020

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Nov 6, 2020

Description

Adds explicit handler and not use express-static for page-data fetch requests. This allow us to find exact page browser request resources for and will enable triggering query on demand.

Side-effect will also be that we won't serve "stale" page-data resources in develop (builds already handle it by deleting files - #26937 )

[ch18448]

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Nov 6, 2020
@pieh pieh added topic: resource loading* and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Nov 6, 2020
Comment on lines +242 to +245
const pageData = await readPageData(
path.join(store.getState().program.directory, `public`),
page.path
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify - we will actually use the new page data utility here? (i.e. #27939)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pieh pieh force-pushed the dev-page-data-express-handler branch from 78d3dba to ee81ad6 Compare November 10, 2020 13:56
Comment on lines +20 to +28
emitter.on(`DELETE_PAGE`, action => {
if (page404 && action.payload.path === page404.path) {
boundActionCreators.deletePage({
...page404,
path: PROD_404_PAGE_PATH,
})
page404 = null
}
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • prod-404 internal plugin is actually also used in gatsby develop
  • removing /404/ page (that this plugin clones into /404.html) wasn't removing clone which meant that 404 page removals didn't actually removed /404.html page

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know much about this plugin, just want to understand: why does it hook into onCreatePage instead of creating its own page? And where do we actually create the source 404 page?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so explanation for this plugin - when user creates src/pages/404.js, the actual path for page is /404/ (that's what gatsby-plugin-page-creator does) which translated to /404/index.html when generating .html file - this is fine, but a lot of hosting solutions handle /404.html as potential default to show 404 page (I think Netlify does this for sure). So we do special case 404 kind of page and create "duplicate" page for it

The src/pages/404.js is also not only case when we do this - if user creates page programatically in createPages - this will work too.

Note that this is mostly needed for production builds, but our shared runtime (both dev and prod) also uses /404.html in multiple places, so the way this plugin works impact both develop and prod.

@pieh pieh changed the title feat(develop): add explicit express handler for page-data requests (WIP) feat(develop): add explicit express handler for page-data requests Nov 12, 2020
@pieh pieh marked this pull request as ready for review November 12, 2020 10:43
@pieh pieh requested a review from a team as a code owner November 12, 2020 10:43
Copy link
Contributor

@vladar vladar left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍 Posted a couple of questions - but that's more out of curiousity. Nothing blocking.

Comment on lines +20 to +28
emitter.on(`DELETE_PAGE`, action => {
if (page404 && action.payload.path === page404.path) {
boundActionCreators.deletePage({
...page404,
path: PROD_404_PAGE_PATH,
})
page404 = null
}
})
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know much about this plugin, just want to understand: why does it hook into onCreatePage instead of creating its own page? And where do we actually create the source 404 page?

@pieh pieh merged commit 3d0de4a into master Nov 12, 2020
@delete-merged-branch delete-merged-branch bot deleted the dev-page-data-express-handler branch November 12, 2020 17:30
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

2 participants