-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
breaking: require nodejs_als
compatibility flag
#14189
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 2c7b63d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
documentation/docs/25-build-and-deploy/60-adapter-cloudflare.md
Outdated
Show resolved
Hide resolved
@@ -120,6 +123,19 @@ You may wish to refer to [Cloudflare's documentation for deploying a SvelteKit s | |||
|
|||
Functions contained in the [`/functions` directory](https://developers.cloudflare.com/pages/functions/routing/) at the project's root will _not_ be included in the deployment. Instead, functions should be implemented as [server endpoints](routing#server) in your SvelteKit app, which is compiled to a [single `_worker.js` file](https://developers.cloudflare.com/pages/functions/advanced-mode/). | |||
|
|||
## Node compatibility | |||
|
|||
SvelteKit uses the [`AsyncLocalStorage`](https://nodejs.org/api/async_context.html#class-asynclocalstorage) API internally. You need to enable the `nodejs_als` [compatibility flag](https://developers.cloudflare.com/workers/runtime-apis/nodejs/), either via the Cloudflare dashboard (if using Pages) or your Wrangler configuration, or your app may break in production. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this says you need to enable it via the dashboard or config file, but it looks like the code will error if it's not in the config file, so maybe we should drop the reference to the dashboard here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh right, I've remembered why I hesitated to put this PR up — it's entirely possible that you've set the flag via the dashboard and so don't need the config file at all. The way this should work is that the framework should be able to configure this stuff on the user's behalf but I don't think that exists on Cloudflare
So maybe it should only error for workers, and be a warning for pages? Ugh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way this should work is that the framework should be able to configure this stuff on the user's behalf but I don't think that exists on Cloudflare
I've talked to the Cloudflare folks and the recommended way to handle this is through a generated Wrangler config: https://developers.cloudflare.com/workers/wrangler/configuration/#generated-wrangler-configuration . I'm not a big fan of it though as it requires duplicating the user's config since it doesn't extend an existing one. Duplicating can also get tricky because any relative paths set need to be changed to resolve from where the generated config is instead of where the original config is.
Co-authored-by: Ben McCann <[email protected]>
@@ -301,6 +301,22 @@ function validate_wrangler_config(config_file = undefined) { | |||
validate_worker_settings(wrangler_config); | |||
} | |||
|
|||
const flags = wrangler_config?.compatibility_flags ?? []; | |||
|
|||
if (flags.includes('nodejs_compat') && flags.includes('nodejs_als')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check that a main
key is specified in the config too so that it's only needed if a user is deploying a Worker. I've met some folks who are only using the Cloudflare adapter for static sites / single-page apps. The docs should also mention something similar
@@ -120,6 +123,19 @@ You may wish to refer to [Cloudflare's documentation for deploying a SvelteKit s | |||
|
|||
Functions contained in the [`/functions` directory](https://developers.cloudflare.com/pages/functions/routing/) at the project's root will _not_ be included in the deployment. Instead, functions should be implemented as [server endpoints](routing#server) in your SvelteKit app, which is compiled to a [single `_worker.js` file](https://developers.cloudflare.com/pages/functions/advanced-mode/). | |||
|
|||
## Node compatibility | |||
|
|||
SvelteKit uses the [`AsyncLocalStorage`](https://nodejs.org/api/async_context.html#class-asynclocalstorage) API internally. You need to enable the `nodejs_als` [compatibility flag](https://developers.cloudflare.com/workers/runtime-apis/nodejs/), either via the Cloudflare dashboard (if using Pages) or your Wrangler configuration, or your app may break in production. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SvelteKit uses the [`AsyncLocalStorage`](https://nodejs.org/api/async_context.html#class-asynclocalstorage) API internally. You need to enable the `nodejs_als` [compatibility flag](https://developers.cloudflare.com/workers/runtime-apis/nodejs/), either via the Cloudflare dashboard (if using Pages) or your Wrangler configuration, or your app may break in production. | |
SvelteKit uses the [`AsyncLocalStorage`](https://nodejs.org/api/async_context.html#class-asynclocalstorage) API internally. You need to enable the `nodejs_als` [compatibility flag](https://developers.cloudflare.com/workers/runtime-apis/nodejs/) via your Wrangler configuration, or your app may break in production. |
It may be better to force Cloudflare Pages users to have a Wrangler configuration since it's required when migrating to Cloudflare Workers anyway. Also makes it easier for us since we can't detect dashboard settings during a local build.
Supersedes #13605.
Honestly I don't know if this guidance is 100% correct — in particular I'm not sure if this contains the right advice around configuration Pages apps (the Workers documentation says 'Cloudflare Pages and Workers Assets is preferred over this approach', while the Pages documentation says 'We recommend using Cloudflare Workers for new projects', so who knows which approach you're meant to use) — but I eventually managed to deploy a test app using the
wrangler
CLI (though unfortunately not via the git integration/dashboard, which is just spinners for me) and confirmed that this flag does allow you to usegetRequestEvent
asynchronously.Erroring on build might be a little aggressive given that most apps don't need
getRequestEvent
synchronously, but it feels better than failing at runtime for the minority of cases that do.Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.Edits