Cloudflare adapter disables known cloudflare worker "compatible" dependencies #263
Comments
|
Wow! thanks for this super thorough analysis and is definitely a real problem, looks like. First step would be to fix the adapter to make things work! Then we should explore how |
|
I think addressing export conditions in solid is the easiest. Libs use solid export and generally don't follow this so real only core is a concern. I was looking and Deno suggested using Part of me wants just to have one The only contention here are people insisting on adding reactivity to the server and not wanting the default server conditions to result to the reactive-less bundles. But that's a problem for later. |
|
I have struggled to find good examples but today I found that |
|
Hi, I tried another experiment today, I tried to see if Vite could build the worker with the correct dependencies in one build step. I messed around with some of the alias settings but couldn't get any of to work correctly. then I moved on to patching the solid-js package.json file. After patching I was able to build an almost Cloudflare-ready worker with the below Vite config. (Side note: If I could figure out how to make the cloudflare entry,js the Vite entry point, I think it could be deployed in Wrangler --nobundle mode) import path from "path";
import solid from "solid-start/vite";
import { defineConfig } from "vite";
export default defineConfig(async (env) => ({
esbuild: false,
plugins: [
solid({
adapter: {
start() {},
async build(config, builder) {
await builder.server(path.join(config.root, ".solid", "server"));
await builder.client(path.join(config.root, "dist", "public"));
},
},
}),
],
...(env.ssrBuild && {
resolve: {
conditions: ["worker"],
},
}),
ssr: {
noExternal: true,
target: "webworker",
},
}));What I want to note is, I got stuck about this Vite config not working for a while. After I patched the "worker" condition into the solid-js package JSON files, I could see the vite resolver logic trying to use it as expected. But it still compiled the browser file into the final SSR bundle. It turns out the secret ingredient is that Vite is also checking the browser map field of package.json. For Vite SSR mode with target=webworker, after it finds the export from the exports in package.json field, it runs it through the browser map, which maps it back to the client version. https://github.com/vitejs/vite/blob/main/packages/vite/src/node/plugins/resolve.ts const targetWeb = !ssr || ssrTarget === 'webworker';
// ...
if (targetWeb && isObject(browserField)) {
entry = mapWithBrowserField(entry, browserField) || entry
}So uh, it may be good to consider remove the Here was my patch to solid-js (deployed using yarn berry): diff --git a/package.json b/package.json
index 79c1d70367baea440cf8092b7da05feb3c6f243f..5a18fd09e135eae82e2d2c7ffd36ffc1a8d68a6e 100644
--- a/package.json
+++ b/package.json
@@ -11,10 +11,6 @@
},
"main": "./dist/server.cjs",
"module": "./dist/server.js",
- "browser": {
- "./dist/server.cjs": "./dist/solid.cjs",
- "./dist/server.js": "./dist/solid.js"
- },
"unpkg": "./dist/solid.cjs",
"types": "types/index.d.ts",
"sideEffects": false,
@@ -44,6 +40,13 @@
],
"exports": {
".": {
+ "worker": {
+ "import": {
+ "types": "./types/index.d.ts",
+ "default": "./dist/server.js"
+ },
+ "require": "./dist/server.cjs"
+ },
"browser": {
"development": {
"import": {
@@ -120,6 +123,13 @@
},
"./store/dist/*": "./store/dist/*",
"./web": {
+ "worker": {
+ "import": {
+ "types": "./web/types/index.d.ts",
+ "default": "./web/dist/server.js"
+ },
+ "require": "./web/dist/server.cjs"
+ },
"browser": {
"development": {
"import": {
diff --git a/web/package.json b/web/package.json
index 4c6dc0a37661bd34435f0ad4666010c5bcb460e9..802ee6512bfb55d4616e2959a2ec84cd23ebef4b 100644
--- a/web/package.json
+++ b/web/package.json
@@ -2,16 +2,19 @@
"name": "solid-js/web",
"main": "./dist/server.cjs",
"module": "./dist/server.js",
- "browser": {
- "./dist/server.cjs": "./dist/web.cjs",
- "./dist/server.js": "./dist/web.js"
- },
"unpkg": "./dist/web.cjs",
"types": "./types/index.d.ts",
"type": "module",
"sideEffects": false,
"exports": {
".": {
+ "worker": {
+ "import": {
+ "types": "./types/index.d.ts",
+ "default": "./dist/server.js"
+ },
+ "require": "./dist/server.cjs"
+ },
"browser": {
"development": {
"import": { |
|
That's annoying and possibly something to be taken up with Vite I suppose. Browser field is important for non-ESM envs. Things like Jest. Legacy systems that don't support export maps. I don't believe it can just be removed. |
|
Sure that makes sense. Especially considering that I just checked and esbuild does not seem to have that quirk. So I guess unless/until that behavior can be changed in Vite it is better to NOT set |
|
okay this seems like such a weird and complicated but one that we need a really nice solution for.. I wonder if @patak-dev can help us here with how we should go about this? |
|
Yeah I am not 100% sure how to go about this too. I think it would be worthwhile to do something because IMO, solid-start has a amazing value proposition for the worker deployment target and it has the potential to capture a huge amount of this space if it is easy to deploy, especially if it is space-efficient. Personally I would suggest first adding some new worker friendly export condition to solid-js core library. To the best of my knowledge this would be "good enough" to let the adapter use esbuild as a second step and create a very nice build for cloudflare workers. I don't know how to decide about condition names like "workers", "deno", "server", To make it less scary, maybe the Solid team could add a really long and clearly experimental condition name like "experimental-solid-start-server" with no guarantee that it will be there forever? Then the solid-start cloudflare adapter could be updated to match this condition. If a new solid-js core package is released with this change, it would also be be easier to create a reproduction of the way Vite is resolving exports/browser fields see if there is any wiggle room for Vite to change the behavior here. (Also I guess it would be good to do a comparison between vite, rollup, esbuild, webpack at this point too) |
|
I tried the solid@1.5.6 which had some new exports added for deno and worker, unfortunately I don't think it really makes a difference yet because the "browser" condition comes first and has priority over the "worker" condition. Would it be possible to move the "worker" condition before the "browser" condition? I've been doing some more research and made a tool to help visualize how the exports are resolved, here is solid-js/web: https://package-exports-analyzer-7jzl0pti8-patdx.vercel.app/solid-js/web?preset=cloudflare (Sorry if there are bugs) Cloudflare Wrangler uses the following options for esbuild:: Since both conditions can be matched, the worker condition needs to come first in the export map of I don't think it is really feasible to drop the "browser" condition/platform from the bundler config too, because these settings encourage modules like I can't find a perfect example, but the best I could find, react-dom and @emotion/react put "worker" above "browser" too:
Interestingly Deno seemed to go for "deno", "node", "import" so it does not check "browser" and is probably fine where it is, though I did not check the CDN bundlers like Skypack, etc. |
|
|
|
Yeah I still have not fully wrapped my head around that part of conditional exports. I like the way aliases work(ed) because as a package consumer I could easily set aliases for some packages I cared about. But with export conditions, basically the bundler only takes one set of conditions that it will use to resolve all the dependencies so it seems like there may always be some whackamole when using global/universal condition names. Recently I noticed this section in webpack's guide and maybe it is the solution: https://webpack.js.org/guides/package-exports/#conditions-custom
You could have a for example |
|
Well changed the order in Solid 1.5.7. Maybe that helps? |
|
@ryansolid Thank you for the update, it definitely helped! I created a new project from scratch and added some of the problematic dependencies like
That said, I still did one major tweak. I changed the wrangler.toml like so: - main = "./dist/server.js"
+ main = "./.solid/server/server.js"This is because the bundled file at I think the solid-start-cloudflare-workers (and page?) adapters may need an update now. How do you prefer to approach this? I can see a few possibilities:
Personally I would suggest 1 because I think it is the simplest. I think there is value in letting Wrangler do the bundle with the actual module resolution because it will improve solid-start compatibility with any existing Cloudflare Workers tutorial. Also it would be faster to build by reducing the number of build steps from 3 to 2. One concern is what happens when referencing solid-js component libraries through node_modules. I will try to test this out. |
|
I saw in the updated solid-start v0.1.2 is trying "Strategy 3". I updated my test repo and the build output by solid-start-cloudflare-worker at So unfortunately wrangler still throws an error when trying to process this file: Note, even using the fallback Wrangler mode I think most of these Node imports are generated by various fetch polyfills for Node (undici/cross-fetch/etc). I think we should support the "browser" (pkg.browser, and pkg.exports[condition=browser])), like Wrangler's esbuild config, to make sure these polyfills do not get included. I tried to patch the builder in separate branch here: https://github.com/patdx/solid-start-bundle-experiment/pull/2/files nodeResolve({
+ browser: true,
preferBuiltins: true,
exportConditions: ["worker", "solid"]
}),This fixed all the node imports ✔️ , and got the right renderToStream function ✔️ however it brought back the "pkg.browser overriding pkg.exports" conflict ❌ , so I had to patch solid-js too. :( So as of the moment, it seems that both Vite and Here are some relevant PRs and Issues from Rollup:
As far as I can tell this was kind of arbitrarily decided, maybe Rollup (and Vite) could be convinced to change their default behavior to always have pkg.exports take priority over pkg.browser based on solid-js's case (preserving backwards compatibility and test compatibility). |
|
Hmm, I did a test of vite/esbuild/rollup/webpack and all of them except esbuild seem to have pkg.browser override pkg.exports https://github.com/patdx/package-exports-test. So that is the more popular rule at the moment. Of course I like esbuild's handling the best and I think it is the most reasonable logic, but does seem to have been a little bit of a lucky coincidence that Wrangler decided to use esbuild, thus it can resolve to all the most ideal imports for Cloudflare Workers environment. |
|
I saw some new tickets created for rollup and vite. In case it may help I created an alternative branch of my last test here, browser-override-issue with the rollup config patch only. This branch demonstrates the failing build. This can be verified by the existence of browser-only code for Solid.js, like |
@patdx if you run esbuild with the |
@guybedford I wonder if we are talking about the same thing or maybe I missed something in my esbuild config? I did a test here: https://github.com/patdx/package-exports-test/blob/main/out/esbuild/simple.js Esbuild seems to resolve to browser.js, so it uses the value from pkg.exports directly. This behavior is preferred for solid-js. But some other bundlers like Vite seem to resolve to browser-override.js. Vite is taking the value resolved from pkg.exports then applying the pkg.browser map on top of it. This is not preferred for solid-js.
|
|
@patdx thanks for putting together this clear test repo, that's certainly information for me. I've posted evanw/esbuild#2593 for further discussion. |
|
There hasn't been much action on this issue so let me try to summarize the current status: The resolution behavior of different bundlers is unchanged since the creation of this issue: https://github.com/patdx/package-exports-test ESBuild still resolves in the way that is most helpful to solid-js project, and the other bundlers tested do not. There have been three related issues kindly created by Ryan and Guy: evanw/esbuild#2593 Unfortunately does not seem to be much traction on them at the moment. I'm not quite sure what to do next or how to get this issue moved closer to a solution. Is there some good way to get it prioritized for discussion? |
|
Cross-linking this issue as seems to be related: |
|
All patches required to make SolidStart work on Cloudflare Workers right now for anyone interested. Use Updated 2023-09-03 for solid-start v0.3.5 diff --git a/package.json b/package.json
index f0a0483df966f0e66eff7c9b88e8b2bfc022ddbb..0b0686e37e5354bcf8ec58940a543423bde7f02b 100644
--- a/package.json
+++ b/package.json
@@ -11,10 +11,6 @@
},
"main": "./dist/server.cjs",
"module": "./dist/server.js",
- "browser": {
- "./dist/server.cjs": "./dist/solid.cjs",
- "./dist/server.js": "./dist/solid.js"
- },
"unpkg": "./dist/solid.cjs",
"types": "types/index.d.ts",
"sideEffects": false,
diff --git a/store/package.json b/store/package.json
index 252dd14b8c3c4b9c2bd84cc9c117176912b5b223..ffd2889042a12c4ca352779ddc84c8f571789060 100644
--- a/store/package.json
+++ b/store/package.json
@@ -2,10 +2,6 @@
"name": "solid-js/store",
"main": "./dist/server.cjs",
"module": "./dist/server.js",
- "browser": {
- "./dist/server.cjs": "./dist/store.cjs",
- "./dist/server.js": "./dist/store.js"
- },
"unpkg": "./dist/store.cjs",
"types": "./types/index.d.ts",
"type": "module",
diff --git a/web/package.json b/web/package.json
index fee7a002f4418f67b2f9f2c6fd202e2c94fc5e21..f4b2a9881a58ecf0f5d3412d6d415e57579e9f74 100644
--- a/web/package.json
+++ b/web/package.json
@@ -2,10 +2,6 @@
"name": "solid-js/web",
"main": "./dist/server.cjs",
"module": "./dist/server.js",
- "browser": {
- "./dist/server.cjs": "./dist/web.cjs",
- "./dist/server.js": "./dist/web.js"
- },
"unpkg": "./dist/web.cjs",
"types": "./types/index.d.ts",
"type": "module",solid-start-cloudflare-workers@0.3.5.patch diff --git a/index.js b/index.js
index 8586eeccb3f41f8d84a1995f3bdb1b7bb793fa33..2cc145a25b887baad9cc9dd303a6adf6a5aa8c45 100644
--- a/index.js
+++ b/index.js
@@ -88,7 +88,7 @@ export default function (miniflareOptions = {}) {
Request,
Response,
fetch,
- crypto,
+ ...(!globalThis.crypto && { crypto }),
Headers,
ReadableStream,
WritableStream,
@@ -197,6 +197,7 @@ export default function (miniflareOptions = {}) {
plugins: [
json(),
nodeResolve({
+ browser: true,
preferBuiltins: true,
exportConditions: ["worker", "solid"]
}),NOTE: the following patch is only required if you want passing typechecks ( diff --git a/api/internalFetch.ts b/api/internalFetch.ts
index eea745dfaa9e31a12a3e22cf343d1c06ee0d7b2f..c541c0df6e9e0c9bfdc8f8ac22da6a92f1fd49b1 100644
--- a/api/internalFetch.ts
+++ b/api/internalFetch.ts
@@ -8,6 +8,7 @@ export const registerApiRoutes = (routes: MatchRoute[]) => {
apiRoutes = routes;
};
+// @ts-ignore
export async function internalFetch(route: string, init: RequestInit, env: Env = {}, locals: Record<string, unknown> = {}) {
if (route.startsWith("http")) {
return await fetch(route, init);
diff --git a/server/middleware.ts b/server/middleware.ts
index b605421621b9636507ac3472add36dbd6702f082..6013558c8cac208b78e5e5780120cc18b424f737 100644
--- a/server/middleware.ts
+++ b/server/middleware.ts
@@ -67,6 +67,7 @@ export const inlineServerFunctions: ServerMiddleware = ({ forward }) => {
JSON.stringify({
url: url.pathname,
entries: entries,
+ // @ts-ignore
...(await serverResponse.json())
})
)
diff --git a/server/render.ts b/server/render.ts
index 8b2e8dc36cbdd36581e5ff98fe716d5fc000f419..313a294e44ef585ea6759b3c2a6226b7f5984a37 100644
--- a/server/render.ts
+++ b/server/render.ts
@@ -68,7 +68,8 @@ export function renderAsync(
let pageEvent = createPageEvent(event);
- let markup = await renderToStringAsync(() => fn(pageEvent), options);
+ // @ts-ignore
+ let markup = await renderToStringAsync(() => fn(pageEvent), event.locals.options);
if (pageEvent.routerContext && pageEvent.routerContext.url) {
return redirect(pageEvent.routerContext.url, {NOTE: another patch only required if you want passing typechecks, see natemoo-re/micromorph#22 diff --git a/package.json b/package.json
index 3251a6ca39f860b6aeb377afff63b57339a4f24c..b167449d14c777a4a644d9bd427400c794656cf1 100644
--- a/package.json
+++ b/package.json
@@ -2,9 +2,12 @@
"name": "micromorph",
"version": "0.4.5",
"main": "./index.mjs",
- "types": "./types.d.ts",
+ "types": "./index.d.ts",
"exports": {
- ".": "./index.mjs",
+ ".": {
+ "default": "./index.mjs",
+ "types": "./index.d.ts"
+ },
"./spa": "./dist/spa.js",
"./nav": "./dist/nav.js"
},package.json "pnpm": {
"overrides": {
"micromorph@<0.4.5": "^0.4.5"
},
"patchedDependencies": {
"solid-js@1.7.11": "patches/solid-js@1.7.11.patch",
"solid-start@0.3.5": "patches/solid-start@0.3.5.patch",
"solid-start-cloudflare-workers@0.3.5": "patches/solid-start-cloudflare-workers@0.3.5.patch",
"micromorph@0.4.5": "patches/micromorph@0.4.5.patch"
}
}
import { defineConfig } from "vite"
import solid from "solid-start/vite"
import cloudflare from "solid-start-cloudflare-workers"
export default defineConfig({
plugins: [
solid({
adapter: cloudflare({
envPath: true,
packagePath: true,
wranglerConfigPath: true,
buildCommand: ""
})
})
]
})
KV values are persisted in two different folders: dev ->
Miniflare v3 was just released https://github.com/cloudflare/miniflare/releases and comes with a long list of breaking API changes https://miniflare.dev/get-started/migrating#api-changes. I did a quick test upgrading Update 2023-09-17 Ryan recently announced #1052 that all adapters will be removed and replaced by https://github.com/unjs/nitro in SolidStart Beta 2. Unfortunately Nitro is also using miniflare v2 at the moment. |
|
In setting up for SolidStarts next Beta Phase built on Nitro and Vinxi we are closing all PRs/Issues that will not be merged due to the system changing. If you feel your issue was closed in mistake. Feel free to re-open it after updating/testing against 0.4.x release. Thank you for your patience. See #1139 for more details. |
Summary
The adapter solid-start-cloudflare-workers pulls in "node" exports of dependencies by default. This makes sense for solid-js framework and possibly precompiled Solid Components in node_modules. However, for many other third-party packages, this will pull in incompatible "node" versions of those libraries. Using browser/web exports when bundling for cloudflare workers (and similar edge runtimes) will probably lead to more success.
Background
I want to set up a graphql server as an API route.
What happened:
1. Plain cloudflare worker test
I tried to set up a graphql server using a package that claims to work with cloudflare.
Here is my implementation: https://github.com/patdx/solid-start-bundle-experiment-old/tree/main/plain-cloudflare-worker
It works great :)
2. Attempted integration into solid-start
I added it to an API route and In solid-start dev/node mode it worked fine.
https://github.com/patdx/solid-start-bundle-experiment-old/tree/main/solid-with-standard-adapter
I tried to build it with the
solid-start-cloudflare-workersadapter.But, I got a really big 2.5mb dist/server.js with a bunch of node imports
Even if I turn on
node_compat=truein my wrangler.toml it still does not work, wrangler fails with an error like:Not to mention there seems to be at least one or two fetch polyfills inside the bundle
3. My attempted fix
I noticed that cloudflare wrangler CLI is automatically bundling with
conditions: ["worker", "browser"]However, the solid-start-cloudflare-workers prebundles with
["node", "solid"]. This step is pulling in the (unwanted) Node.js version@graphql-yoga/commonand all sorts of extra dependencies and Node polyfills.I tried to give wrangler the
.solid/sever.jsdirectly and let wrangler do the only bundling instead.This looked like it would work, until I went to try running the code and it errored out because that
renderToStringAsyncis not available in the browser. Oh, oops 😅After some trial and error, I found I could set resolveOnly: "solid-js" inside of my fork of solid-start-cloudflare-workers. Then, this step bundled the node/server version of solid-js so renderToStringAsync can work. And then wrangler bundled everything else in the browser friendly version. It worked! :)
Conclusion
Edge runtime need the server version of solid-js.
However, for many other packages, this will pull in polyfills and node-native code that are very heavy or do not work in an edge runtime. Not to mention, I think it will be a bit confusing for developers to find that solid-start-cloudflare-workers only supports a subset of the packages supported by plain cloudflare workers.
I've tried to survey some other adapters and it seems most of the are leaning towards targeting web/webworker environments.
Additionally, some Vite-based frameworks do this inside Vite itself, saving a bundle step. For example, Shopify Hydrogen.
In a perfect world, I think we would try to find a way to get solid-js to expose its server exports to worker environments naturally. Perhaps using the
workerexport condition like emotion css. Then, solid-start-cloudflare-workers could target a worker/browser environment to be consistent with other cloudflare deploy pipelines while still getting the server exports from solid-js. This would also benefit non-solid-start based SSR environments that target cloudflare workers. However, theworkerexport is not really a standard, I don't know if this will be enough to support all edge runtimes.I found this old discussion on the main repo: solidjs/solid#308 (reply in thread)
Which is kind of related too. Exposing a parallel import "server" seems like it could be nice too (but would not be enough for compatibility with plain cloudflare wrangler API).
If it is preferred to solve by only changing the adapter, maybe we can add some configuration settings or consider using my
hackofresolveOnly: "solid-js", so the adapter is only bundling solid-js and wrangler is handling the majority of the dependencies. Note: I guess any solution may need special logic to handle solid components imported from node_modules too. (Not sure how to do this...)Related links
The text was updated successfully, but these errors were encountered: