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: preview fallback #11312

Merged
merged 3 commits into from
Dec 12, 2022
Merged

fix: preview fallback #11312

merged 3 commits into from
Dec 12, 2022

Conversation

sapphi-red
Copy link
Member

@sapphi-red sapphi-red commented Dec 11, 2022

Description

Alternative to #11289

As #11289 was controversial, this PR tries to focus on #10925.
This PR patches sirv as suggested by @bluwy (#11289 (comment)).

I still think we should avoid patching for maintainability. But I left this as a patch as it was easier and I think it's ok for a short-term solution.

fixes #10925
superseds close #10944

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.

@sapphi-red sapphi-red added p4-important Violate documented behavior or significantly improves performance (priority) regression The issue only appears after a new release labels Dec 11, 2022
@bluwy
Copy link
Member

bluwy commented Dec 11, 2022

Nice! Should we also update

const serve = sirv(dir, sirvOptions(headers))
// Keep the named function. The name is visible in debug logs via `DEBUG=connect:dispatcher ...`
return function viteServePublicMiddleware(req, res, next) {
// skip import request and internal requests `/@fs/ /@vite-client` etc...
if (isImportRequest(req.url!) || isInternalRequest(req.url!)) {
return next()
}
if (shouldServe(req.url!, dir)) {
return serve(req, res, next)
}
next()
}

here too? That should make the sirv flow like before https://github.com/vitejs/vite/pull/10475/files#diff-035e162040589730e5d3f09533d13b458d86742db439aebab46bed9ed5179941L55-R59

@patak-dev
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Dec 11, 2022

📝 Ran ecosystem CI: Open

suite result
astro ✅ success
histoire ✅ success
iles ✅ success
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt-framework ✅ success
previewjs ✅ success
rakkas ✅ success
sveltekit ❌ failure
vite-plugin-ssr ✅ success
vite-plugin-react ✅ success
vite-plugin-react-swc ✅ success
vite-plugin-svelte ❌ failure
vite-plugin-vue ✅ success
vite-setup-catalogue ✅ success
vitepress ✅ success
vitest ❌ failure
windicss ✅ success

@patak-dev
Copy link
Member

Vitest should be failing because they merged and deleted the vite-4 branch to main (sent a PR to update ecosystem-ci here vitejs/vite-ecosystem-ci#177)

@danielroe did you change something on Nuxt side? If not, this PR seems to be fixing vite-4 support for you.

@dominikg @benmccann I think we should also merge and release this one in the next patch. The issue with vite-plugin-svelte and SvelteKit may be related to the same problem we see in the other PR I tagged you into. Could you also confirm that we can merge this without affecting your plans?

@danielroe
Copy link
Contributor

The fix is the new rollup release which addressed the issue we were experiencing - the branch is green in Nuxt ci as well 😊

@patak-dev
Copy link
Member

Awesome! It looks like we are back to full green then! 🥳

@benmccann
Copy link
Collaborator

It looks like the SvelteKit CI is failing generally and affecting both PRs. We've got a tool called turborepo that's supposed to help with caching, but the cache invalidation seems to be broken somehow and so a commit snuck by and broke our build because it looked green when we merged it, but it actually broke things. Dominik has the caching turned off on the ecosystem CI, so we're seeing the failure here. I'll ping Rich and Simon to see if they can get the build green again or see if I get anytime to look at it later tonight. I'd be a lot more comfortable putting out a new release if we can get the build green first

The vite-plugin-svelte issue is a different one. @dominikg sent a fix for it: sveltejs/vite-plugin-svelte#546

@patak-dev
Copy link
Member

Thanks @benmccann, we are not going to release until seeing SvelteKit green, dont worry 👍

@dominikg
Copy link
Contributor

there was an actual issue hidden behind the problems of svelte's ecosystem ci runs failing due to kit changes.
After vite-plugin-svelte suite was fixed on our end, it still failed

this should fix it #11335

mentioning here as it also involves preview

@patak-dev
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Dec 12, 2022

📝 Ran ecosystem CI: Open

suite result
astro ✅ success
histoire ✅ success
iles ✅ success
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt-framework ✅ success
previewjs ✅ success
rakkas ✅ success
sveltekit ✅ success
vite-plugin-ssr ✅ success
vite-plugin-react ✅ success
vite-plugin-react-swc ✅ success
vite-plugin-svelte ✅ success
vite-plugin-vue ✅ success
vite-setup-catalogue ✅ success
vitepress ✅ success
vitest ✅ success
windicss ✅ success

@patak-dev patak-dev merged commit cfedf9c into vitejs:main Dec 12, 2022
@sapphi-red sapphi-red deleted the fix/preview-fallback branch December 13, 2022 11:44
futurGH pushed a commit to futurGH/vite that referenced this pull request Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p4-important Violate documented behavior or significantly improves performance (priority) regression The issue only appears after a new release
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
6 participants