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

Allow falling back to index page in SpaRouter when serving on / #933

Closed
dnaka91 opened this issue Apr 13, 2022 · 15 comments
Closed

Allow falling back to index page in SpaRouter when serving on / #933

dnaka91 opened this issue Apr 13, 2022 · 15 comments

Comments

@dnaka91
Copy link

dnaka91 commented Apr 13, 2022

Feature Request

Motivation

Wanted to replace our current solution in Trunk for SPA apps with the new SpaRouter from axum-extra. This would allow us to drop our current implementation, done with hyper-staticfile.

Unfortunately, I noticed that when serving the SpaRouter at the root /, it doesn't fall back to index.html anymore.

In Trunk the usual setup is to have the index page and additional assets all in the same folder. So without custom config it would always result in SpaRouter::new("/", "dist"). That makes the router consider everything to be a 404 as it is considered inside the assets folder.

Instead, we need the router to fall back to the index.html, same as when using the router on anything else but the root.

Proposal

The SpaRouter is technically correct in its implementation, but I see the root / rather as a special case. Therefore, I see 2 solutions.

  1. Check that the serve route is / and enable the fallback to index.html on all missing files.

  2. Add a new option that allows to override the 404 logic, always returning the index.html on missing files, no matter what.

Alternatives

@dnaka91 dnaka91 changed the title Allow falling back to index page in SpaRouter when serving on/ Allow falling back to index page in SpaRouter when serving on / Apr 13, 2022
@davidpdrsn
Copy link
Member

Are you saying you're serving assets from the root path? So like /style.css rather than /assets/style.css? I've never done that (rails doesn't) but some people have brought it up. I suppose it could make sense to support.

@dnaka91
Copy link
Author

dnaka91 commented Apr 18, 2022

Exactly that. In our case, we compile a project to wasm, including other web-related files. Then we output everything in one single folder and serve that one locally. So the assets and the index.html are all together in one single folder (plus sub-folders eventually), and represent the whole frontend app.

I already had a look at the PR you prepared so quickly, still have to test this out. While looking at the code, I realized that my initial statement is a bit wrong.

We don't just need to serve from the root, but technically need this “always fall back to index.html” behavior pretty much everywhere.

So if the root changes through a user flag, it changes the whole root of where the wasm app is served. But the problem remains, that we have the index.html and all assets together and the same root where the actual app is served. Therefore, we technically move the whole root.

I hope it isn't asked too much, to turn this feature into a flag, that can be passed to the SpaRouter and allows to unconditionally fall back to index.html?

@davidpdrsn
Copy link
Member

davidpdrsn commented Apr 18, 2022

We don't just need to serve from the root, but technically need this “always fall back to index.html” behavior pretty much everywhere.

Isn't that what my change in the PR does?

Router::new()
    .route("/foo", get(|| async { "GET /foo" }))
    .merge(SpaRouter::new("/", "dist"));
  • /foo goes to the handler.
  • / looks for an asset at /, doesn't find any and returns the index file.
  • Same for any other path.

If not then can you provide some examples of how you'd like things routed.

@dnaka91
Copy link
Author

dnaka91 commented Apr 20, 2022

Sorry, let me give you an example to make it a bit more clear. So we have basically an SPA router + eventually proxies (forwarding a specific route to some other server/service).

The base version would be this:

Router::new()
    .fallback(Router::from(SpaRouter::new(public_route, &state.dist_dir))
    .route("/_trunk/ws", get(...));

The /_trunk/ws route is always there and coupled to some injected frontend code. It allows us to refresh the browser page, once compilation is done and new assets are available.

Additional proxies might be added to the router later, if configured by the user. They just forward a configured URL to a backend server, which allows for local test setup where an external service is expected by the frontend at a given route.

By default, the public_route is "/" and &state.dist_dir is "dist". These can be configured, though.

So the default turns into SpaRouter::new("/", "dist"), but based on the configuration we might end up with SpaRouter::new("/app", "dist") as well.

In that case, we'd still want to have the same logic as if the route is "/", as the whole app moves. With the current PR changes, it'd still be the old behavior as before for such route settings.

Therefore, it would be great to have a flag in the SpaRouter that makes it always fall back to index.html, regardless of the set route.

@davidpdrsn
Copy link
Member

Ah I see! That makes sense 😃 I'll try and implement that in my PR.

@davidpdrsn
Copy link
Member

@dnaka91 I've added SpaRouter::serve_index_file_on_missing_asset to #941. Wanna give it a go and see if it works for you?

@dnaka91
Copy link
Author

dnaka91 commented Apr 20, 2022

Sure, thank you so much 👍.

Will have to wait until tomorrow though, already quite late for me here.

@dnaka91
Copy link
Author

dnaka91 commented Apr 21, 2022

Hm, I guess we almost have it. For my test, I set up the SpaRouter as follows:

SpaRouter::new("/app", "dist")
    .serve_index_file_on_missing_asset(true)

This works great now for getting the index file from any location, but it seems to work a bit too good 😅. So for example /app, /app/something, /app/deep/route/me all give me the index file, but I even get it for /meep, which should not be the case (for us).

I think that is the right behavior for the SpaRouter in regard to how it worked before: serve assets from the asset dir, fall back to the index otherwise. Now as we move the whole app with that public route, it would be odd to have it match on other routes outside the configured route.

I tried to fix this myself with:

Router::new()
    .nest(
        public_route,
        Router::from(
            SpaRouter::new(public_route, &state.dist_dir)
                .serve_index_file_on_missing_asset(true),
        ),
    )

But that seems not allowed, as the SPA router uses a fallback and that doesn't work with nest. So we'd need yet another option on it, to make it serve only from within the configured route and fallback inside of it, but not outside.

At this point, I wonder whether I should maybe write my own router for this. Slowly get the feeling that our case is a bit to specialized for the average user of the SpaRouter type 🤔

@dnaka91
Copy link
Author

dnaka91 commented Apr 21, 2022

For reference, here is the PR on our side, trying out to move to SpaRouter: trunk-rs/trunk#349

@dnaka91
Copy link
Author

dnaka91 commented Apr 21, 2022

I have a few ideas how to make the whole SpaRouter maybe more flexible overall. After all, it's more or less just a ServeDir + ServeFile as fallback. Also, I think the call to fallback could give surprises when merged with another router.

Was thinking whether it would be better to let the user decide how to build the SPA logic and make the actual SpaRouter just the dir + index fallback logic.

Will experiment a bit around with this idea and come back to you.

@davidpdrsn
Copy link
Member

What is /meep? You haven't mentioned that until now.

Router::new()
    .nest(
        public_route,
        Router::from(
            SpaRouter::new(public_route, &state.dist_dir)
                .serve_index_file_on_missing_asset(true),
        ),
    )

Doesn't work for serving assets at the root (ie when public_route is /). That calls .nest("/", _) which overlaps with all other routes meaning users cannot add routes of their own. In this case SpaRouter is not just ServeDir + ServeFile.

Also, the intended usecase is Router::new().merge(SpaRouter::new(...)), not .nest(...). That makes a bunch of things simpler because SpaRouter can add its own fallback via merge which it cannot do via nest.

@davidpdrsn
Copy link
Member

@dnaka91 have you tested my PR? And seen what I wrote above?

@dnaka91
Copy link
Author

dnaka91 commented Apr 24, 2022

Hey sorry, had quite a packed weekend. I'll get to it tomorrow.

Only missing piece that I would need is to not have the fallback enabled... I think.

@davidpdrsn
Copy link
Member

I think we should leave SpaRouter as it is. tower-http's ServeDir recently got built in fallback support so you no longer need to write middleware to handle missing files and so building your own custom SPA routing is much easier.

@dnaka91
Copy link
Author

dnaka91 commented Apr 28, 2022

I totally agree ❤️

At first, I thought I'd be great to add these features to SpaRouter, but while we were discussing it further and further, I realized, this case is rather specific and the SpaRouter is just a simple wrapper over ServeDir + ServeFile. So I'm better off just implementing that part directly with tower-http.

Thank you for pointing out the new fallback support in tower-http. I didn't know about it, and this seems to be exactly what I need in Trunk.

Thanks again for all the effort you put into this issue. Axum is a great framework and I really appreciate your support in this (and other issues/questions I had in the past) 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants