Skip to content

fix: Add fallback for spa in preview.#10944

Closed
fc wants to merge 7 commits intovitejs:mainfrom
fc:clay/preview-spa-fallback
Closed

fix: Add fallback for spa in preview.#10944
fc wants to merge 7 commits intovitejs:mainfrom
fc:clay/preview-spa-fallback

Conversation

@fc
Copy link
Contributor

@fc fc commented Nov 15, 2022

Description

fix #10925

Resolves this issue - Routing broken for preview with react-router-dom in 4.0.0-alpha.2

When config.appType === 'spa' is true then then the single option of sirv is enabled. Docs - "When true, the directory's index page (default index.html) will be sent if the request asset does not exist."
So, I think we would want to mimic the behavior of sirv for spa apps and to fallback to routing to index.html when shouldServe is false.

Questions - is there an appropriate way to handle the tests for this? It's blowing up.

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.

@fc fc force-pushed the clay/preview-spa-fallback branch from 30f4e79 to 38f919a Compare November 15, 2022 18:37
@fc fc marked this pull request as ready for review November 15, 2022 18:47
@bluwy
Copy link
Member

bluwy commented Nov 18, 2022

We would need to update the failing tests too. Looks like 2 we're incorrectly checking this for the SPA case.

@bluwy bluwy added the p3-significant High priority enhancement (priority) label Nov 18, 2022
@fc
Copy link
Contributor Author

fc commented Nov 22, 2022

@bluwy:

We would need to update the failing tests too. Looks like 2 we're incorrectly checking this for the SPA case.

I took some stabs at updating the tests. Currently it defaults to being an appType of spa but ultimately we want to check for both spa/mpa cases so we can have coverage of both scenarios.

@fc fc force-pushed the clay/preview-spa-fallback branch from 18901c5 to a56c139 Compare November 23, 2022 18:31
@fc fc force-pushed the clay/preview-spa-fallback branch from 7e03965 to 5136f11 Compare November 23, 2022 18:59
})

test('CSS dependencies are tracked for HMR', async () => {
await startDefaultServe('spa')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not 100% sure why this is necessary.

Comment on lines +81 to +87
'index: 404',
'index-legacy: 404',
'chunk-async: 404',
'chunk-async-legacy: 404',
'immutable-chunk: 404',
'immutable-chunk-legacy: 404',
'polyfills-legacy: 404'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why this would be 404s?

@fc
Copy link
Contributor Author

fc commented Nov 23, 2022

I don't understand why my tests fail on Linux but not on macOS
https://github.com/vitejs/vite/actions/runs/3535108279/jobs/5932757683#step:12:20

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

Labels

p3-significant High priority enhancement (priority)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Routing broken for preview with react-router-dom in 4.0.0-alpha.2

2 participants