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

fix: $ vite preview 404 handling #7665

Merged
merged 1 commit into from
Apr 12, 2022
Merged

fix: $ vite preview 404 handling #7665

merged 1 commit into from
Apr 12, 2022

Conversation

brillout
Copy link
Contributor

Description

When running a production preview with $ vite preview, Vite fallbacks 404 pages to the landing page /. See the dirv option single:

opts.single
Default: false

Treat the directory as a single-page application.

When true, the directory's index page (default index.html) will be sent if the request asset does not exist.

Currently this option is set to true and this PR changes it to false.

I guess Vite used to assume that every Vite app was an SPA, but that's not really the case anymore.

For MPA and SSR apps, having a 404 page be the landing page / is quite an unexpected behavior.

We could make this a config, but I don't think it's worth it. I think it's fine if $ vite preview serves SPAs only at /. (I would even argue that it's better, as it's a more truthful representation and preview of production.)

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@patak-dev
Copy link
Member

Maybe we should have an appType: 'spa' | 'mpa' config (probably not with this name or options values but you get the idea)? The problem is that by default we have the SPA fallback middleware during DEV, and IIUC it is assumed the user will configure the deployment service to do the same:

export function spaFallbackMiddleware(

Maybe the option should also remove the spa fallback from dev for example. @benmccann, are you also removing this middleware by hand, no?

@brillout
Copy link
Contributor Author

I don't know of any SPA that is deployed to multiple URLs. For example https://www.photopea.com/ is only deployed to / and https://www.photopea.com/doesNotExist gives a 404. Deploying SPAs to a single URL is the norm and it makes sense for SEO (in order to consolidate the Google "page rank" to as few URLs as possible).

Considering this, I don't see the added value of having such SPA fallback. Or am I missing something?

If we don't see the added value, I'd argue we should remove the SPA fallback altogether.

@bluwy
Copy link
Member

bluwy commented Apr 11, 2022

For MPAs, maybe it makes sense for vite preview to return a special 404.html page if it exists, or fallback to / by default. But I'm not sure how vite preview is being used for SSR here. Following Vite's SSR playground, SSR apps are previewed via node server.js, and it makes sense because there are many ways SSR setups can output the build. It's also the reason SvelteKit doesn't use Vite's preview server.

So I think vite preview handling SSR could be out-of-scope at the moment, but definitely worth exploring a way to converge on handling SSR apps.

@brillout
Copy link
Contributor Author

But I'm not sure how vite preview is being used for SSR here.

vps injects a SSR middleware using configureServer and configurePreviewServer().

This enables vps users to simply run $ vite dev / $ vite build / $ vite preview and things just work (e.g. github.com/brillout/vite-plugin-ssr/examples/react/package.json).

This is not only for vps users but also for vps frameworks I am / folks are building. (E.g. Vilay which is a Vite + vps + Relay framework.)

For MPAs, maybe it makes sense for vite preview to return a special 404.html page if it exists

👍 Good idea. This is what Static Hosts actually do.

or fallback to / by default.

I'm reluctant about this. It's quite an uncommon behavior. I don't see why $ vite preview should add behavior that doesn't exist in production. After all, the goal of $ view preview is to be a truthful representation of production.

@brillout brillout changed the title fix: set sirv option single to false fix: $ vite preview 404 handling Apr 11, 2022
@brillout
Copy link
Contributor Author

For MPAs, maybe it makes sense for vite preview to return a special 404.html page if it exists

👍 Good idea. This is what Static Hosts actually do.

Done.

@patak-dev patak-dev added this to the 3.0 milestone Apr 11, 2022
@patak-dev patak-dev added the p2-to-be-discussed Enhancement under consideration (priority) label Apr 11, 2022
@brillout
Copy link
Contributor Author

I'm seeing this is marked for the 3.0 release. While this does change Vite's behavior, it's not really a breaking change since it won't break production apps and won't break development. It's more of a fix really and we can release it in a minor.

Note that the current behavior is quite weird for SSR apps. Imagine you make a typo in the URL and then you see your landing page, that's weird and makes it seem like a bug (I actually dug at the wrong place when this first occurred to me). I think it's bad enough to postpone vps's support for Vite's CLI until this is fixed.

I think the PR's current state is good to be merged. But let me know if there are any questions.

@bluwy
Copy link
Member

bluwy commented Apr 11, 2022

I'm reluctant about this. It's quite an uncommon behavior. I don't see why $ vite preview should add behavior that doesn't exist in production. After all, the goal of $ view preview is to be a truthful representation of production.

Checking Vite's guide for static deployment, it looks like only the Google Firebase guide showed how to rewrite the path for SPAs. So yeah I think it make sense to remove single: true 👍

In the future maybe we could explore ways to simplify redirects if the user wants to configure them.

@patak-dev patak-dev removed this from the 3.0 milestone Apr 11, 2022
@benmccann
Copy link
Collaborator

@benmccann, are you also removing this middleware by hand, no?

Yes, we are removing this middleware. I recently created #7631 discussing it

This enables vps users to simply run $ vite dev / $ vite build / $ vite preview and things just work

Do you mean vite build && vite build --ssr instead of vite build? If not, I'm curious how you do it without building multiple times

@brillout
Copy link
Contributor Author

@patak-dev
Copy link
Member

patak-dev commented Apr 11, 2022

Copy link
Collaborator

@benmccann benmccann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. I agree this wouldn't necessarily have to wait until 3.0

@brillout I'm curious, what will handle SSR when you use Vite's preview server?

@patak-dev patak-dev merged commit 66b6dc5 into vitejs:main Apr 12, 2022
@cexbrayat
Copy link

FYI: this is maybe more breaking than expected, as all apps using create-vue (recommended setup for Vue 3) are relying on preview for e2e tests. (see https://github.com/vuejs/create-vue/blob/da5c870106d0585a7df22f4b715688cc5a7a2c3d/template/config/cypress/package.json)

This means e2e tests start breaking when bumping vite to v2.9.2 if they do something like cy.visit('/login').

I think this change should be listed as breaking?
Is there at least a way to configure this behavior (I did not find a way when scrolling through the docs)?

@brillout
Copy link
Contributor Author

@cexbrayat Are you sure your problem is related to this PR? If your /login page doesn't exist, why would you expect anything else than a 404?

@brillout
Copy link
Contributor Author

@benmccann See #7658.

@patak-dev
Copy link
Member

@cexbrayat @brillout if this is really breaking, we should revert and in any case evaluate it for v3.

@cexbrayat
Copy link

@brillout because it's a Vue 3 SPA, and create-vue uses preview as a server that approximates what a production server would do (redirect to index.html).

If there is no way to configure this option directly, maybe this should be reverted, as I suppose a lot Vue 3 users (those who write e2e tests) will run into this.

@brillout
Copy link
Contributor Author

production server would do (redirect to index.html).

Precisely, that's not the case. See #7665 (comment).

As for tests: if your /login doesn't exist, why would you test it?

I suppose a lot Vue 3 users (those who write e2e tests) will run into this.

I don't see that for the same reason: why would users test a page that doesn't exist?

@benmccann
Copy link
Collaborator

I'm not a Vue user, but I think I understand what's going on here. I think the idea is that the server is configured to respond with index.html to all requests. /login is a route that the client-side router knows how to handle and it can then render the login page. This makes it so that a SPA can be served on a simple html server that only serves static content. If a route does not exist then the client side router will display the 404 page (though I'm guessing the server will still return a 200 status in this case).

I think Vue's behavior is not a good default, but we probably shouldn't change it in 2.9. I would say the behavior with this PR is a better default and then in 3.0 we can make Vue's behavior behind a flag like --single

patak-dev added a commit that referenced this pull request Apr 13, 2022
@patak-dev patak-dev mentioned this pull request Apr 13, 2022
4 tasks
@brillout
Copy link
Contributor Author

brillout commented Apr 13, 2022

I see.

@patak-dev I'm 👍 for reverting.

I'll re-create this PR with a flag --single which I'll set to true by default so we can merge and release it for 2.9.

For 3.0 we can then change the default to false.

@patak-dev
Copy link
Member

Thanks @brillout, we can discuss there the specifics. The flag may need to be global if it would affect the dev and preview server. It could also end up removing the SPA fallback middleware from the dev server that @benmccann is looking to get rid of by default.

patak-dev added a commit that referenced this pull request Apr 13, 2022
@brillout
Copy link
Contributor Author

@patak-dev 👍 Sounds like a plan.

@patak-dev
Copy link
Member

@cexbrayat thanks for the quick feedback! vite@2.9.3 has been released reverting this PR

@cexbrayat
Copy link

@brillout Sorry if I was not clear: most JS frameworks aiming at building a SPA offer a router that takes care of the navigation client-side. So when you deploy your app, you configure your server to serve index.html for every request. Then the router does its thing and /login displays the login page. That's why you can write e2e tests for the /login URL. This is what preview was doing previously: maybe create-vue should not use this for e2e tests, and we can totally change it (I'm a member of the Vue team). But in the meantime, the change was slightly surprising in a patch release.

Thanks @patak-dev for the quick revert and release ♥️

@brillout
Copy link
Contributor Author

That makes a lot of sense. Thanks for the explanation @cexbrayat @benmccann.

@brillout
Copy link
Contributor Author

brillout commented May 7, 2022

This PR is resurrected at #8061.

@ktym4a
Copy link

ktym4a commented Feb 4, 2024

Is still have this problem?
I mean it would be good to Vite 5 to return a special 404.html page if it exists with appType = 'mpa'.

When I set appType = 'mpa', Vite sets notFoundMiddleware.
so If I want to only custom 404 behavior, do I need to set appType = 'custom' to handle or create my own Middleware?

@brillout brillout deleted the fix/sirv branch September 18, 2024 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-to-be-discussed Enhancement under consideration (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants