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(cloudflare): resolve conditions for svelte #487

Closed

Conversation

rognstadragnar
Copy link

@rognstadragnar rognstadragnar commented Dec 16, 2024

Changes

Fixes #485.

#476 added the workerd and worker resolve conditions to fix the Vue build breaking. As this fix used the defaultClientConditions as a base it broke builds using Svelte. The client default conditions include the browser condition, which makes Svelte use the client entry when server side rendering.

I feel that the defaultServerConditions make the most sense as a default here, and with filtering out the node condition the Vue build will still work.

Testing

Added usage of setContext and onMount to the Cloudflare Svelte fixture.
Without this fix the tests then fail. With the fix all tests pass.

Docs

Should not really affect anything as this is just a bug fix.

Copy link

changeset-bot bot commented Dec 16, 2024

🦋 Changeset detected

Latest commit: 7ed4460

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 12 packages
Name Type
@astrojs/cloudflare Patch
@test/astro-cloudflare-astro-dev-platform Patch
@test/astro-cloudflare-astro-env Patch
@test/astro-cloudflare-compile-image-service Patch
@test/astro-cloudflare-external-image-service Patch
@test/astro-cloudflare-wasm Patch
@test/astro-cloudflare-no-output Patch
@test/astro-cloudflare-routes-json Patch
@test/astro-cloudflare-with-solid-js Patch
@test/astro-cloudflare-with-svelte Patch
@test/astro-cloudflare-with-vue Patch
@test/astro-cloudflare-wrangler-preview-platform Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@rognstadragnar rognstadragnar changed the title fix: cloudflare resolve conditions for svelte fix(cloudflare): resolve conditions for svelte Dec 16, 2024
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

I guess with #476, #410 resurfaced again and hasn't really been properly fixed. However, the behaviour now returns to the same as Vite 5 (and the previous major of the cloudflare adapter).

However, I don't know if this is the correct fix. We can't remove both "browser" and "node" conditions, either one needs to be enabled. I think I have to think a bit more about how to fix this first. There's an inconsistency where we're trying to use resolve conditions for Cloudflare (webworkers), but during dev, we're actually still running in node.

Blocking this for now but you can pin to the previous version that worked to workaround this at the meantime.

@alexanderniebuhr
Copy link
Member

@bluwy I feel like this is a good enough workaround for now. Are there any hard reasons not to merge this?

@bluwy
Copy link
Member

bluwy commented Dec 19, 2024

I prefer to do a proper fix once than constantly changing the conditions between releases, because merging this will also cause unexpected issues for already working libraries before

@bluwy
Copy link
Member

bluwy commented Dec 25, 2024

Closing this as fixed by sveltejs/svelte#14779 (#485 (comment))

@bluwy bluwy closed this Dec 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Astro + Svelte + Cloudflare] Cannot build Svelte component that contains $onMount() with Cloudflare adapter
4 participants