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: Overrides SvelteKit version to v1 for the melt-ui test #13

Closed

Conversation

AdrianGonz97
Copy link

@AdrianGonz97 AdrianGonz97 commented Jan 5, 2024

This PR adds a temporary override, pinning SvelteKit to v1 until Melt upgrades to SK v2. If temporarily pinning the version goes against the concept of ecosystem-ci, please feel free to close!


It looks like Melt keeps failing because it hasn't been upgraded to SK v2 yet. The current error shown in CI is due to this change: https://discord.com/channels/457912077277855764/1185088342929449080/1185230781359063040 which I've now reverted.

However, another error has cropped back up (the same error that presented in the linked discord thread above). While testing ecosystem-ci locally, I'm noticing that we're now failing due to import { vitePreprocess } from '@sveltejs/kit/vite'; no longer being a valid export as it was removed in SK v2.

The interesting bit is that Skeleton is still passing despite not being on SK v2 either. This leads me to believe that the dependencies for Skeleton aren't being replaced like they are for Melt. My best guess as to why that is is that Skeleton is in a monorepo and Melt isn't.

To test that this isn't just an isolated incident, I also added bits-ui and svelte-ux locally to see how they would hold up comparatively. bits-ui errored, just like Melt, and svelte-ux didn't, just like Skeleton. Both, Skeleton and svelte-ux use monorepos, while bits-ui and Melt do not.

Edit: Raised an issue for the monorepo deps discrepancy as this behavior doesn't seem to be intended.

@dominikg
Copy link
Member

dominikg commented Feb 4, 2024

I would prefer if melt-ui would update their tests to use sveltekit 2.
Unlike vite-ecosystem-ci where there is one "upstream" repo , in svelte-ecosystem-ci we can have more, eg svelte+sveltekit. Now usually sveltekit is just a regular dependency and updating svelte4 should work just fine with sveltekit@1+vite-plugin-svelte@2 so instead we should try to find out what adds sveltekit 2 to projects that don't use an override for it.

@dominikg
Copy link
Member

dominikg commented Feb 4, 2024

looks like i did not remember these lines correctly:

repoOverrides[p] === true || (deps.has(p) && repoOverrides[p] == null)
so all packages in "builds" are overriden by default if they are detected in the deps or the project. you can prevent it by explicitly setting an override to false in the config. I wonder if we need to change this behavior. If i remember it correctly it was mainly used while plugin vue and plugin react were still part of vite core repo so it made sense for vite-ecosystem-ci

@AdrianGonz97
Copy link
Author

I would prefer if melt-ui would update their tests to use sveltekit 2.

Melt has now upgraded to SvelteKit 2, so the CI should be green on the next run.

so all packages in "builds" are overriden by default if they are detected in the deps or the project.

Ah, that makes sense and matches the behavior we were seeing with non-monorepo projects.

you can prevent it by explicitly setting an override to false in the config.

Good to know in the future! I was wondering what setting to false did when I first saw it in one of the other tests.

I wonder if we need to change this behavior. If i remember it correctly it was mainly used while plugin vue and plugin react were still part of vite core repo so it made sense for vite-ecosystem-ci

I think it should be changed as it's quite unexpected and will probably break again (in non-monorepo projects) during the next major version of svelte/sveltekit.


Closing this PR as Melt has upgraded to SK 2

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

Successfully merging this pull request may close these issues.

None yet

2 participants