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

refactor: remove CJS ssr output format #13944

Merged
merged 7 commits into from
Sep 17, 2023
Merged

Conversation

patak-dev
Copy link
Member

Description

We're gathering feedback about the future removal of CJS SSR Format here:

This PR implements the removal to show the complexity that we'll be eliminating.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@stackblitz
Copy link

stackblitz bot commented Jul 24, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@patak-dev patak-dev added feat: ssr p3-significant High priority enhancement (priority) labels Jul 24, 2023
@patak-dev patak-dev added this to the 5.0 milestone Jul 24, 2023
@patak-dev
Copy link
Member Author

/ecosystem-ci run

@vite-ecosystem-ci

This comment was marked as outdated.

@patak-dev
Copy link
Member Author

Vue projects are failing because of this conditional in vite-plugin-vue.

Qwik has a conditional on ssr.format here.

@patak-dev
Copy link
Member Author

/ecosystem-ci run

@vite-ecosystem-ci

This comment was marked as outdated.

@patak-dev
Copy link
Member Author

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Aug 10, 2023

📝 Ran ecosystem CI: Open

suite result
analogjs ✅ success
astro ✅ success
histoire ✅ success
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt ❌ failure
nx ✅ success
previewjs ✅ success
qwik ❌ failure
rakkas ✅ success
sveltekit ✅ success
unocss ❌ failure
vite-plugin-pwa ✅ success
vite-plugin-ssr ❌ failure
vite-plugin-react ✅ success
vite-plugin-react-pages ❌ failure
vite-plugin-react-swc ✅ success
vite-plugin-svelte ❌ failure
vite-plugin-vue ❌ failure
vite-setup-catalogue ✅ success
vitepress ❌ failure
vitest ✅ success

@patak-dev
Copy link
Member Author

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Sep 14, 2023

📝 Ran ecosystem CI: Open

suite result
analogjs ✅ success
astro ✅ success
histoire ❌ failure
ladle ❌ failure
laravel ✅ success
marko ✅ success
nuxt ✅ success
nx ✅ success
previewjs ✅ success
qwik ✅ success
rakkas ✅ success
sveltekit ✅ success
unocss ✅ success
vite-plugin-pwa ✅ success
vite-plugin-ssr ✅ success
vite-plugin-react ✅ success
vite-plugin-react-pages ❌ failure
vite-plugin-react-swc ✅ success
vite-plugin-svelte ❌ failure
vite-plugin-vue ✅ success
vite-setup-catalogue ✅ success
vitepress ✅ success
vitest ✅ success

@patak-dev
Copy link
Member Author

Added back the types temporarily so we can test ecosystem CI, see results here (same errors as in main). The change is reverted and only the legacy empty object is kept.

@sapphi-red
Copy link
Member

Oops the fix foo comment closed this PR 😅

@sapphi-red sapphi-red reopened this Sep 15, 2023
@sapphi-red
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Sep 15, 2023

📝 Ran ecosystem CI: Open

suite result
analogjs ✅ success
astro ✅ success
histoire ❌ failure
ladle ❌ failure
laravel ❌ failure
marko ✅ success
nuxt ✅ success
nx ✅ success
previewjs ✅ success
qwik ❌ failure
rakkas ✅ success
sveltekit ✅ success
unocss ✅ success
vite-plugin-pwa ✅ success
vite-plugin-ssr ✅ success
vite-plugin-react ✅ success
vite-plugin-react-pages ✅ success
vite-plugin-react-swc ✅ success
vite-plugin-svelte ❌ failure
vite-plugin-vue ✅ success
vite-setup-catalogue ✅ success
vitepress ❌ failure
vitest ✅ success

bluwy
bluwy previously approved these changes Sep 15, 2023
@sapphi-red
Copy link
Member

sapphi-red commented Sep 15, 2023

It seems laravel is failing with an error that could be a bug.
https://github.com/vitejs/vite-ecosystem-ci/actions/runs/6195012334/job/16820400780#step:8:390

I reran once so I guess it's not a flaky fail

@patak-dev
Copy link
Member Author

patak-dev commented Sep 15, 2023

@dominikg, did you say the laravel fail happens in main too after updating vite-ecosystem-ci? I see it working here though: https://github.com/vitejs/vite-ecosystem-ci/actions/runs/6194105986/job/16816620515

laravel/vite-plugin doesn't seems to be using this feature: https://github.com/search?q=repo%3Alaravel%2Fvite-plugin+buildSsrCjsExternalHeuristics&type=pullrequests

@sapphi-red
Copy link
Member

sapphi-red commented Sep 15, 2023

laravel/vite-plugin uses vitest ^0.25.2 (0.25.8) and that might be the problem.
0.25.8 is released in 2022/12 and I won't be surprised if it's not compatible with Vite 5.

@sapphi-red
Copy link
Member

I removed some more unused codes.

@sapphi-red
Copy link
Member

I ran ecosystem-ci locally and the fail reproduced. After upgrading Vitest, the tests passed, so I created a PR: laravel/vite-plugin#246
I guess Vitest was relying on CJS ssr output format in old versions.

@bluwy bluwy merged commit 2f60b9e into main Sep 17, 2023
5 checks passed
@bluwy bluwy deleted the refactor/remove-cjs-ssr-format branch September 17, 2023 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: ssr p3-significant High priority enhancement (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants