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

Add --spa option #772

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

johannesloetzsch
Copy link

Allows serving of SPAs

  • with correct status code 200
  • without using the defekt Catch-all redirect
Relevant issues

fixes issue #757

Contributor checklist
  • Provide tests for the changes (unless documentation-only)
  • Documented any new features, CLI switches, etc. (if applicable)
    • Server --help output
    • README.md
    • doc/http-server.1 (use the same format as other entries)
  • The pull request is being made against the master branch
Maintainer checklist
  • Assign a version triage tag
  • Approve tests if applicable

Allows serving of SPAs
* with correct status code 200
* without using the defekt `Catch-all redirect`

fixes issue http-party#757
@thornjad
Copy link
Member

thornjad commented Dec 6, 2021

SPA support has been discussed numerous times over the years. See especially #369 (comment), but there's further discussion in #369, #405 and #194 (there are a few others, but they didn't add much to the discussion).

Although this PR is small, I'm concerned that adding an --spa option would open the door to needing to support all the rest of the functionality of an SPA.

Does this PR add anything other than an approach to solving #757?

@johannesloetzsch
Copy link
Author

Thank you @thornjad for linking the related issues :)

My motivation was solving the trouble related to #757. I don't insist in this pull request to be the chosen solution. All options I commented would be fine for me.

My reasoning for implementing an --spa option is, that the currently in the documentation suggested workarounds (404.html and Catch-all redirect) are either not conform to the http standard or come with an unnecessary performance overhead.
This pull request shows, that an correct implementation can be shorter (only counted the changes to lib/core/index.js, not the boilerplate for option parsing) than the description of how to apply the workarounds at README.md.

I would argue, the amount of attempts to implement such an option show, that already a minimal solution would be very appreciated. Unfortunately the inked alternatives are all unmaintained.

You wrote:

I'm concerned that adding an --spa option would open the door to needing to support all the rest of the functionality of an SPA.

Could you elaborate what other functionality for serving an SPA might be requested?

@dartess
Copy link

dartess commented Mar 11, 2022

I personally do not care what this function is called. If it's called page404 or otherwise, I'll be happy to use it too. This is more convenient than copying the index file and renaming it every time.

@johannesloetzsch
Copy link
Author

I personally do not care what this function is called. If it's called page404 or otherwise, I'll be happy to use it too. This is more convenient than copying the index file and renaming it every time.

There is another major advantage of using the solution from this pull request, compared to the 404.html workaround: The --spa option returns the correct HTTP status code. This is relevant for CI, Monitoring and other interaction via scripts.

@Tails
Copy link

Tails commented Apr 29, 2022

+1 could greatly use an SPA-functionality.

Need http-server as opposed to 'serve' because http-server supports Range Requests in bytes, which are needed for loading a static sqlite3 db on-demand using wa-sqlite.

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