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

fix(gatsby): Vendor express-static so we can avoid loading html files from public during development #12336

Merged
merged 5 commits into from
Mar 8, 2019

Conversation

KyleAMathews
Copy link
Contributor

@KyleAMathews KyleAMathews commented Mar 5, 2019

Fixes #12243 (review)

Works locally for reactjs.org which is a site that uses paths like
/something/about-us.html

screenshot 2019-03-05 14 18 11

@KyleAMathews KyleAMathews requested a review from a team as a code owner March 5, 2019 22:20
var path = parseUrl(req).pathname
const parsedPath = sysPath.parse(path)

// Ignore html files.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what I added

@KyleAMathews KyleAMathews changed the title Vendor express-static so we can not load html files fix(gatsby): Vendor express-static so we can not load html files Mar 5, 2019
@KyleAMathews KyleAMathews changed the title fix(gatsby): Vendor express-static so we can not load html files fix(gatsby): Vendor express-static so we can avoid loading html files from public during development Mar 5, 2019
@KyleAMathews
Copy link
Contributor Author

We have some development server tests? This would be easy to test. Write out a html file to public and then request it and a regular path from the Dev server and verify the two html files are the same.

@wardpeet
Copy link
Contributor

wardpeet commented Mar 7, 2019

How do we feel about the following code?
This way we don't need to keep serve-static in sync, we just do the html check ourselves. rest is handled by static-serve.

const sysPath = require(`path`)
const express = require(`express`)
const parseUrl = require(`parseurl`)

/**
 * Module exports.
 * @public
 */

module.exports = serveStatic

/**
 * @param {string} root
 * @param {object} [options]
 * @return {function}
 * @public
 */

function serveStatic(root, options) {
  const expressStatic = express.static(root, options)

  return function(req, res, next) {
    if ([`GET`, `HEAD`].includes(req.method)) {
      const path = parseUrl(req).pathname
      const parsedPath = sysPath.parse(path)
      if ([`.htm`, `.html`].includes(parsedPath.ext)) {
        return next()
      }
    }

    return expressStatic(req, res, next)
  }
}

I tested it, seems to work.
I ran gatsby build later I did gatsby-develop and files were updating

@DSchau
Copy link
Contributor

DSchau commented Mar 7, 2019

@KyleAMathews @wardpeet that seems like a really good solution! It’s nice to not have to patch and maintain things—so if that works the same we should definitely go that route!

@KyleAMathews
Copy link
Contributor Author

Yeah that'd be much better 👌

@KyleAMathews
Copy link
Contributor Author

Want to just take over the PR?

@sidharthachatterjee
Copy link
Contributor

Went ahead and made Ward's changes!

Copy link
Contributor Author

@KyleAMathews KyleAMathews left a comment

Choose a reason for hiding this comment

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

💯

Copy link
Contributor

@DSchau DSchau 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 - let's get this merged! 💪

@DSchau DSchau merged commit 800e023 into master Mar 8, 2019
@DSchau DSchau deleted the fix-html-serving branch March 8, 2019 15:09
@DSchau
Copy link
Contributor

DSchau commented Mar 8, 2019

Successfully published:

  • gatsby@2.1.27

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

4 participants