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-index option #515

Merged

Conversation

sinking-point
Copy link

Closes #474

@aliemjay
Copy link
Contributor

aliemjay commented May 3, 2021

Good job!
This would be even easier to implement with actix/actix-web#2135

Now I wonder if --spa-index and --index options should be compatible or if there is any use case for both of them together?
If not, then I think it is better to have a --spa flag instead of --spa-index. In this way, we avoid conflicting command line options. So then the usage would be something like:
$ miniserve --spa --index ./index.html

@sinking-point
Copy link
Author

Thanks. I'm aware of the PR, but those changes aren't yet released and I needed this feature sooner rather than later.

Hmm, your suggestion is certainly more intuitive. But if you want to have an SPA that is not at the root of your site then having the flexibility of my implementation would be useful.

@sinking-point
Copy link
Author

That is something of an edge case though. I can change it to be how you suggested if you like.

@aliemjay
Copy link
Contributor

aliemjay commented May 6, 2021

Well, I don't have a strong opinion on this because I don't use SPAs!

I think we need a second opinion @svenstaro .

@svenstaro
Copy link
Owner

Is the behavior of the current --index as you would expect for your needs, @sinking-point? There were a ton of changes to miniserve behavior recently and this might just be fixed for your use case now.

@sinking-point
Copy link
Author

@svenstaro I have just tried with the latest version and unless I'm missing something the problem is the same. Is there a specific change that you expected to resolve this?

@sinking-point sinking-point force-pushed the sinkingpoint/add-spa-index-option-474 branch from dec6959 to 6dfe4d1 Compare October 8, 2021 16:09
src/main.rs Outdated Show resolved Hide resolved
@svenstaro
Copy link
Owner

@sinking-point can you fix the CI thing real quick? It's just a small formatting issue.

@sinking-point
Copy link
Author

@svenstaro sure, should be fine now.

@svenstaro
Copy link
Owner

Clippy doesn't seem happy yet. :)

@svenstaro
Copy link
Owner

@sinking-point Can you fix this real quick?

@sinking-point
Copy link
Author

@svenstaro my bad, I didn't realise there was a linter to contend with. Hopefully sorted now.

@svenstaro
Copy link
Owner

Cool, merging as is but I'll likely make some more ergonomics changes.

@svenstaro svenstaro merged commit e1c5d47 into svenstaro:master Oct 25, 2021
@sinking-point
Copy link
Author

Perfect, thanks for merging.

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.

[feature request] add an ability to serve SPA (single page application) app
3 participants