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 utils/develop path extension check to support location.search #1844

Merged
merged 2 commits into from
Aug 19, 2017

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Aug 17, 2017

This PR adds the ability to define a client-side route that properly supports a dynamic querystring (location.search) in development mode.

For example

exports.onCreatePage = async ({ page, boundActionCreators }) => {
  const { createPage } = boundActionCreators;

  return new Promise((resolve, reject) => {
    if (someCriteriaCheck) {
      page.matchPath = 'path.html:args?';

      createPage(page);
    }

    resolve();
  })
};
URL Works before? Works after?
localhost:8000/path.html
localhost:8000/path.html/foo
localhost:8000/path.html/?foo
localhost:8000/path.html?foo

More context available in this discussion thread.

I have not yet tested the production build of the Gatsby site I'm working on but I assume this will work there as-is. (If not I'll follow up with another PR.)

bvaughn added a commit to bvaughn/react that referenced this pull request Aug 17, 2017
Note that client-side routing won't work correctly until the following Gatsby PR has landed: gatsbyjs/gatsby#1844
@gatsbybot
Copy link
Collaborator

gatsbybot commented Aug 17, 2017

Deploy preview ready!

Built with commit 3455ff9

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

@@ -108,7 +108,7 @@ async function startServer(program) {
// Render an HTML page and serve it.
app.use((req, res, next) => {
const parsedPath = parsePath(req.originalUrl)
if (parsedPath.extname === `` || parsedPath.extname === `.html`) {
if (parsedPath.extname === `` || parsedPath.extname.indexOf(`.html`) === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

extname.startsWith()? might be a bit clearer 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay 😛 I guess both are used in this project.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, we are slowly trying reach some sort level consistency on that stuff. Thanks for the change, i realize it's small and a bit inane to have asked 🙇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah, it's a great goal. Thanks for pointing it out! 😄

@gatsbybot
Copy link
Collaborator

gatsbybot commented Aug 17, 2017

Deploy preview ready!

Built with commit 3455ff9

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

@KyleAMathews
Copy link
Contributor

So an alternative way to handle this I've been thinking is to just delete all html files from public when starting bootstrap. The reason this check is here is because without it, you'd load html from previous static builds and wonder why changes weren't hot reloading. If we just deleted all html then we could safely load any path in public as all html requests would be handled by webpack-dev-server.

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 17, 2017

That sounds reasonable.

It seems we could still merge this now and just remove it later if/when you decide to do that though? It would fix a current bug / pain point and it wouldn't (I don't think) cause any regression or negative side effect.

bvaughn added a commit to bvaughn/react that referenced this pull request Aug 17, 2017
@KyleAMathews
Copy link
Contributor

Ah nvm, deleting all html files solves a different problem :-) Merging! Thanks for the PR!

@KyleAMathews KyleAMathews merged commit 05204d3 into gatsbyjs:master Aug 19, 2017
@bvaughn bvaughn deleted the fix-develop-server-path-ext branch August 19, 2017 01:12
@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 19, 2017

aww yasssss 🕺

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.

4 participants