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

feat: add "worker" exports condition #14779

Merged
merged 1 commit into from
Dec 19, 2024
Merged

feat: add "worker" exports condition #14779

merged 1 commit into from
Dec 19, 2024

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Dec 19, 2024

This PR adds the worker condition for entrypoints that exports dual browser/non-browser files, so that worker resolves to the non-browser file.

Reason

In Svelte 5, there has been issues using it in Astro and Cloudflare (withastro/adapters#485). The cloudflare integration configures Vite with ssr.target: 'webworker', which also changes the resolve conditions to browser, development|production, etc.

Additional info: about why `browser` condition is used

Usually if a library have either browser or node conditions, it's more likely for browser to work in those worker-based environments, like Cloudflare's workerd or Vercel edge since they don't support node-based APIs. Though today these environments do have some form of node-based compat APIs, the decision to prefer browser was made before that.

However, using the browser condition in Svelte seem to interfere with the runtime lifecycle, and causes runtime issues (linked issue above). Using the non-browser condition (usually default in the exports) works. This PR adds the worker condition to route to the default file.

Additional info: about why `worker` condition is used

The worker condition had been the usual way (afaict) to refer to "all edge environments". Otherwise you'd have to manually specify workerd, edge-light, any specific runtimes etc. It was touched on at vitejs/vite#6401, and supported in Astro (1, 2) and Solid too.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.
  • If this PR changes code within packages/svelte/src, add a changeset (npx changeset).

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

I also tested this change in Astro and it fixes the issue.

Copy link

changeset-bot bot commented Dec 19, 2024

🦋 Changeset detected

Latest commit: 19047fc

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

This PR includes changesets to release 1 package
Name Type
svelte Minor

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

Copy link
Contributor

Playground

pnpm add https://pkg.pr.new/svelte@14779

@Rich-Harris
Copy link
Member

The whole thing feels very duct-tapey but the rationale makes sense! Here's hoping this doesn't have any unanticipated consequences 😆

@Rich-Harris Rich-Harris merged commit 72b7bc7 into main Dec 19, 2024
11 checks passed
@Rich-Harris Rich-Harris deleted the worker-condition branch December 19, 2024 22:07
@github-actions github-actions bot mentioned this pull request Dec 19, 2024
@Tihomir971
Copy link

Now, doesn't work on Cloudfalre:

11:48:17.641	✘ [ERROR] Expected identifier but found "import"
11:48:17.641	
11:48:17.642	    (define name):1:0:
11:48:17.642	      1 │ import.meta.dirname
11:48:17.642	        ╵ ~~~~~~
11:48:17.642	
11:48:17.642	✘ [ERROR] Expected identifier but found "import"
11:48:17.642	
11:48:17.642	    (define name):1:0:
11:48:17.642	      1 │ import.meta.filename
11:48:17.643	        ╵ ~~~~~~
11:48:17.643	
11:48:17.644	✘ [ERROR] Expected identifier but found "import"
11:48:17.644	
11:48:17.644	    (define name):1:0:
11:48:17.644	      1 │ import.meta.url
11:48:17.645	        ╵ ~~~~~~
11:48:17.645	
11:48:17.645	failed to load config from /opt/buildhome/repo/vite.config.ts
11:48:17.645	error during build:

@Rich-Harris
Copy link
Member

jesus christ. computers were a mistake.

mind opening a fresh issue?

@bluwy
Copy link
Member Author

bluwy commented Dec 20, 2024

That's unrelated to this PR: evanw/esbuild#4010

@Tihomir971
Copy link

That's unrelated to this PR: evanw/esbuild#4010

Maybe, I'm not senior, but when i downgrade to 5.14.6 it works again. Local build work with 5..15.0. Only Cloudfalre don't work with 5.15.0 for me

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.

3 participants