Default routing seems broken (nginx 404 errors)

[mattpetrie]

Hi @mars 😃 !!

I think the readme section that covers configuring nginx routing in static.json could be clarified a bit to emphasize the need to include a "routes" configuration in to avoid nginx 404 responses for non-root routes.

The readme frames the issue in the context of avoiding use of hash-based URLs. What the readme doesn't make clear is that when using "real" URLs, you will get a 404 response from nginx for all non-root URLs if you don't add a routes config to your static.json. In other words, the need to include this configuration exists regardless of whether you are using hash-based routes or not.

Given that it has become increasingly common to use real URLs from the beginning of a project (such as with react-router-dom's BrowserRouter, and getting the 404 page for route that is expected to exist is a pretty serious failure, perhaps it would be good to update the documentation to place more emphasis on needing to add routes to static.json to avoid this pitfall?

When reading the readme I had skipped over the Routing clean URLs section because I was already using real URLs so I assumed it did not apply to my use case, when in fact it was still important for me to add the route config. It also looks like I'm not the first to run into this from misreading the docs.

Happy to submit a suggested change if that helps!

[mars]

Hi @mattpetrie 😍

Thank you so much for bringing this up.

I've lightly contemplated changing the default behavior to always route non-existent paths to index.html, the React app, and have naturally done exactly that in a descendent experiment of this buildpack.

I propose changing the default static.json to "Route clean URLs" (a poor naming choice indeed):

{
  "root": "build/",
  "routes": {
    "/**": "index.html"
  }
}

Basically the "Routing clean URLs" README section will be replaced by a new "Routing" section that describes the new default and how to customize it with static.json.

Is there any reason likely this would break someone's deployed app? I think it's more likely to magically make many apps that use a client-side router work better.

I'm very cautious to make changes to this buildpack that could break any of the thousands of existing apps, so have perhaps unreasonably feared switching this default.

[mars]

This new default behavior can be tested using the buildpack branch:

heroku buildpacks:set https://github.com/mars/create-react-app-buildpack#default-routing

After testing, set the app back to the mainline:

heroku buildpacks:set mars/create-react-app

[mattpetrie]

Hi @mars, sorry to have been so slow to get back to you! I tried to test out the updated buildpack, but when trying to heroku buildpacks:set https://github.com/mars/create-react-app-buildpack#default-routing I get an error, no matches found: https://github.com/mars/create-react-app-buildpack#default-routing.

That said, I think the updated documentation is much more clear! I also think that while there's always some risk of changes breaking someone's custom configuration, this is a change that is likely to support the most common use cases, and I think the documentation makes it clear where to make the change if you do want to diverge from the default.

Thanks for making the quick update, and thanks for making this awesome buildpack!! It really makes it ridiculously quick and easy to get a React app up and running on Heroku without having to fiddle with build process configuration. 👏 👏

Hope you are doing well!

[mars]

Great to hear @mattpetrie I will merge & release this soon, thanks to you catalyzing the issue 🙇‍♂️⚡️💁‍♂️

[chiunhau]

Save my life.

[mars]

🎄🎁