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

Call serveStatic before serving index.html #29

Merged
merged 4 commits into from
Sep 13, 2016

Conversation

geelen
Copy link
Contributor

@geelen geelen commented Sep 6, 2016

This simplifies the implementation, since you no longer need to maintain a list of file type extensions. Now, if a file is found by serveStatic, it'll be served. If not, fall back to index.html (or whatever file you set).

The advantage here is that if you do have static html files in the directory, they'll be served straight up. Which is super handy if you're mixing-and-matching pushstate and static pages, or if you're doing some React prerendering.

I think this is a more expected behaviour, and should make things easier to maintain in the long run (since you don't have to keep adding file extensions to the connect-modrewrite list)

@scottcorgan
Copy link
Owner

That's a great package and a change I want to see made. This package started as just an experiment to get something working years ago and I guess people started using it.

With that said, I'd love to make this change with tests (which are there are none of at the moment). Any way I could get you to add some tests for this kind of change?

Thanks!

@scottcorgan
Copy link
Owner

@geelen I've added the initial tests in master. They're not all there, but there's enough I think to prove that this PR passes. Go ahead and rebase and let me know.

@geelen
Copy link
Contributor Author

geelen commented Sep 8, 2016

@scottcorgan done.

@scottcorgan scottcorgan merged commit d338323 into scottcorgan:master Sep 13, 2016
@scottcorgan
Copy link
Owner

Published as 2.0.0, among other upgrades. Thanks again!

@geelen geelen deleted the patch-2 branch September 26, 2016 04:56
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

2 participants