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.

Sign up for GitHub

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

Jump to bottom

Replace 'serve' package w/ express-static to restore compression #7139

Merged
merged 2 commits into from
Aug 8, 2018
Merged

Replace 'serve' package w/ express-static to restore compression #7139

merged 2 commits into from
Aug 8, 2018

Conversation

KyleAMathews
Copy link
Contributor

fixes #7119

@KyleAMathews
Copy link
Contributor Author

KyleAMathews commented Aug 8, 2018 •
edited
Loading

Deploy preview for using-postcss-sass failed.

Built with commit fbe5bf4

https://app.netlify.com/sites/using-postcss-sass/deploys/5b6ab7e73813f0035e25e922

@KyleAMathews
Copy link
Contributor Author

Sorry about the Prettier noise

KyleAMathews
KyleAMathews commented Aug 8, 2018
View reviewed changes
packages/gatsby/src/commands/serve.js Outdated
@@ -3,18 +3,18 @@ const handler = require(`serve-handler`)
const openurl = require(`opn`)
const http = require(`http`)
const signalExit = require(`signal-exit`)
const compression = require(`compression`)
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 file has the actual changes

@gatsbybot
Copy link
Collaborator

gatsbybot commented Aug 8, 2018 •
edited
Loading

Deploy preview for using-drupal ready!

Built with commit fbe5bf4

https://deploy-preview-7139--using-drupal.netlify.com

@gatsbybot
Copy link
Collaborator

gatsbybot commented Aug 8, 2018 •
edited
Loading

Deploy preview for gatsbygram ready!

Built with commit fbe5bf4

https://deploy-preview-7139--gatsbygram.netlify.com

m-allanson
m-allanson previously approved these changes Aug 8, 2018
View reviewed changes
Copy link
Contributor

@m-allanson m-allanson left a comment

Choose a reason for hiding this comment

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

👍

@m-allanson
Copy link
Contributor

Oh - remove serve from package.json?

m-allanson
m-allanson reviewed Aug 8, 2018
View reviewed changes
packages/gatsby/src/commands/serve.js
@@ -3,18 +3,18 @@ const handler = require(`serve-handler`)
const openurl = require(`opn`)
Copy link
Contributor

Choose a reason for hiding this comment

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

and remove Serve's serve-handler here?

@m-allanson
Copy link
Contributor

Should also:

fix #5184
fix #6671

tryzniak
tryzniak reviewed Aug 8, 2018
View reviewed changes
packages/gatsby/src/commands/serve.js Outdated
@@ -3,18 +3,18 @@ const handler = require(`serve-handler`)
const openurl = require(`opn`)
const http = require(`http`)
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed I guess

@m-allanson
Copy link
Contributor

Could add pathPrefix handling in here to fix #5271?

#5271 (comment)

@azdanov
Copy link
Contributor

azdanov commented Aug 8, 2018

I was experimenting on my own with http-server.

Is this viable alternative? It supports quite a few options out of the box.

serve.js 
/* @flow weak */
const openurl = require(`opn`)
const signalExit = require(`signal-exit`)
const { createServer } = require(`http-server/lib/http-server`)

module.exports = program => {
  let { port, open } = program
  port = typeof port === `string` ? parseInt(port, 10) : port

  let server = createServer({ root: `public` })

  server.listen(port, () => {
    let openUrlString = `http://localhost:` + port
    console.log(`gatsby serve running at:`, openUrlString)
    if (open) {
      let openUrlString = `http://localhost:` + port
      console.log(`Opening browser...`)
      openurl(openUrlString)
    }
  })

  signalExit((code, signal) => {
    server.close()
  })
}

@KyleAMathews
Copy link
Contributor Author

It seems like it'd work great too! I'm not overly worried about which one we use as they're essentially all the same thing running the same node http server underneath.

@KyleAMathews KyleAMathews merged commit ce3d509 into master Aug 8, 2018
@KyleAMathews KyleAMathews deleted the express-serve branch August 8, 2018 09:49
This was referenced Aug 8, 2018
Closed
Closed
@KyleAMathews
Copy link
Contributor Author

Deploy preview for gatsbyjs failed.

Built with commit 9cb60d4

https://app.netlify.com/sites/gatsbyjs/deploys/5b6ab2f8c6aed62b02841c13

m-allanson added a commit that referenced this pull request Aug 8, 2018
@m-allanson
Merge branch 'master' into lerna-fix
42fa817 
* master: (21 commits)
  Disable www-graphql-docker-push.sh (#7127)
  chore(release): Publish
  Fix onRouteUpdate & onPreRouteUpdate (#7142)
  chore(release): Publish
  Fix showing dev 404 page when there isn't one in development (#7140)
  Replace 'serve' package w/ express-static to restore compression (#7139)
  Fix tests (#7138)
  Add delay to cyress test where cyress kept failing
  chore(release): Publish
  (#6581): v2 docs building apps with gatsby overview update (#7115)
  Added my blog to sites (#7125)
  Add babel-plugin-macros (#7129)
  Update babel.md (#7012)
  (#6590): flesh out preparing your environment overview stub (#7137)
  [gatsby-plugin-netlify] update static cache header to match docs (#7132)
  (#7118): Clarify prettier instructions and update tutorial sidebar (#7130)
  Fixing spelling error (#7136)
  chore(release): Publish
  Try publishing again
  chore(release): Publish
  ...

# Conflicts:
#	yarn.lock
m-allanson added a commit that referenced this pull request Aug 8, 2018
@m-allanson
Merge branch 'master' into graphql-codemod
ef3bb8e 
* master: (566 commits)
  chore(release): Publish
  Fix mistakenly identifying files as using commonjs when they weren't (#7153)
  Update collectPackages to getPackages for new lerna rc (#7126)
  Update index.md (#7145)
  Disable www-graphql-docker-push.sh (#7127)
  chore(release): Publish
  Fix onRouteUpdate & onPreRouteUpdate (#7142)
  chore(release): Publish
  Fix showing dev 404 page when there isn't one in development (#7140)
  Replace 'serve' package w/ express-static to restore compression (#7139)
  Fix tests (#7138)
  Add delay to cyress test where cyress kept failing
  chore(release): Publish
  (#6581): v2 docs building apps with gatsby overview update (#7115)
  Added my blog to sites (#7125)
  Add babel-plugin-macros (#7129)
  Update babel.md (#7012)
  (#6590): flesh out preparing your environment overview stub (#7137)
  [gatsby-plugin-netlify] update static cache header to match docs (#7132)
  (#7118): Clarify prettier instructions and update tutorial sidebar (#7130)
  ...

# Conflicts:
#	yarn.lock
porfirioribeiro pushed a commit to porfirioribeiro/gatsby that referenced this pull request Aug 22, 2018
@KyleAMathews @porfirioribeiro
Replace 'serve' package w/ express-static to restore compression (gat…
e4de1b5 
…sbyjs#7139)

* Replace 'serve' package w/ express-static to restore compression

fixes gatsbyjs#7119

* Remove serve
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m-allanson m-allanson m-allanson left review comments

@tryzniak tryzniak tryzniak left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Replace "serve" package w/ express for "gatsby serve"
5 participants
@KyleAMathews @gatsbybot @m-allanson @azdanov @tryzniak