gatsbyjs / gatsby-docker

Docker image that builds and hosts a Gatsby site
MIT License
356 stars 88 forks source link

Fix try_files in order to match gatsby's file structure #7

Closed mfeltscher closed 6 years ago

mfeltscher commented 6 years ago

This fixes an issue where a HTTP error 500 is being shown when trying to access site build.

KyleAMathews commented 6 years ago

Not an Nginx expert but… but isn't the index.html file looked for automatically? Not sure I understand what this change does.

mfeltscher commented 6 years ago

@KyleAMathews: This solution allows you to access /page as well as /page/. I don't know if this should be possible at all but I'm sure you can tell me :) If only the first case (/page) should be supported I can adjust the pull request accordingly.

mfeltscher commented 6 years ago

@KyleAMathews Any news on this?

cusspvz commented 6 years ago

What about the =404? Is it for having the page 404.html?

cusspvz commented 6 years ago

I think the 404.html file resolution on try_files is important.

Relative with the thing you're trying to fix, I have it configured on a website and it works for both cases.

without the last slash with the last slash

mfeltscher commented 6 years ago

The 404 file handling is covered by #8 which I opened a couple of days ago.

About your examples: The first example (without the trailing slash) does a redirect to path + / which for my use case doesn't work. I configured gatsby to not use any trailing slashes and therefore I want all my URLs to not contain any trailing slash.

After thinking about this for a little longer I actually realised that I might make sense to make this configurable. Wdyt?

cusspvz commented 6 years ago

Could you please test with those configs if it works flawless?

mfeltscher commented 6 years ago

I don't think this is what we want to achieve since you would fall back to index.html for a non-existing page instead of showing a proper 404 error.

I adjusted your changes and added a new redirect rule which can be configured based on your need (default is to add trailing slashes since from what I saw this is what Gatsby is using). Could you have a look at this?

mfeltscher commented 6 years ago

@cusspvz Have you found time to test this?

cusspvz commented 6 years ago

@mfeltscher No i didn't, sorry. Can you just show up with cases to explain why the rewrite rule has to have an environment variable?

mfeltscher commented 6 years ago

It's basically just to prevent duplicated content. Basically the following cases exist:

Since we cannot know how your Gatsby application is being built we have to have this variable to make it configurable.

cusspvz commented 6 years ago

Nice!! Is too much to ask you to add that explaination on the readme as Options Environment Variables ?

mfeltscher commented 6 years ago

Sure, makes total sense. I also added documentation for the rest of the environment variables in 5a67457. Would be glad if you could cross-check if I got everything right.

See https://github.com/mfeltscher/gatsby-docker/blob/5a674576d035e7fdcc4b57f1b5f28e78afd5742e/README.md

mfeltscher commented 6 years ago

@cusspvz Have you found the time to look at the README yet?

cusspvz commented 6 years ago

Yes, when I did I was checking if this would build automatically and then I forgot to merge. Thanks for noticing. You did a great job over here!! Thanks!

mfeltscher commented 6 years ago

No worries :)

Thanks for merging!

mfeltscher commented 6 years ago

@cusspvz Is there any chance you could update the latest tag of the Docker image so we have all these changes in there?

cusspvz commented 6 years ago

Travis CI was integrated with the REPO, so we should have tags for PRs and the master updated. ;)

mjackson commented 6 years ago

we should have tags for PRs and the master updated

@cusspvz What does that mean? Is there a master tag now?

mfeltscher commented 6 years ago

@cusspvz Perfect, thx! Btw, there is an easier way to do this using automated builds on Docker Hub :-)

EDIT: Forget about this, just saw that you're also doing linting - great!

mfeltscher commented 6 years ago

@cusspvz For some reason the onbuild tag doesn't get updated - see https://hub.docker.com/r/gatsbyjs/gatsby/tags/ Could you fix this as well?