start-node build uses different node-externals logic than dev

[katywings]

IS: During npm run build the start-node adapter currently runs three build processes, one if them is a pure rollup build.

1. That rollup build doesn't have vite's external-dependency detection magic
2. it also ignores vites config ssr.external (code).

SHOULD:

1. Ideallly that rollup build should have the same external dependency detection as vite (its a bad dev workflow if the dev/prod builds behave inconsistent)
2. Atleast there should be a way for the user to manually set node externals consistently across dev and prod builds.

REPRODUCTION:

• Essentially: use a node-only package (like redis) and run npm run build
• Example

MY WORKAROUND:
I locally cloned start-node and made the following changes. This allows me to set externals in vite's ssr.external config consistently across dev/build.

diff --git a/packages/start-node/index.js b/packages/start-node/index.js
index 599c4bc..c1a189c 100644
--- a/packages/start-node/index.js
+++ b/packages/start-node/index.js
@@ -13,6 +13,7 @@ export default function () {
       import(pathToFileURL(join(config.root, "dist", "index.js")));
     },
     async build(config) {
+      const ssrExternal = config?.ssr?.external || [];
       const __dirname = dirname(fileURLToPath(import.meta.url));
       const appRoot = config.solidOptions.appRoot;
       await vite.build({
@@ -33,6 +34,7 @@ export default function () {
           outDir: "./.solid/server",
           rollupOptions: {
             input: resolve(join(config.root, appRoot, `entry-server`)),
+            external: ssrExternal,
             output: {
               format: "esm"
             }
@@ -54,7 +56,7 @@ export default function () {
           }),
           common()
         ],
-        external: ["undici", "stream/web", "@prisma/client"]
+        external: ["undici", "stream/web", "@prisma/client", ...ssrExternal]
       });
       // or write the bundle to disk
       await bundle.write({ format: "esm", dir: join(config.root, "dist") });

Somewhat related: #95

[ryansolid]

That's not bad. I wonder if we aren't supposed to externalize pretty much everything since if Vite didn't pull it in we don't need to. This is really about getting the entry working.

[katywings]

The one reason coming to my mind, against externalizing everything for the node build is: dev workflow / habits / standards even? 🤔. I imagine that many frontend devs install all of their dependencies as dev dependencies with the logic, that those dependencies are not needed in node_modules after client bundling. (also Docker image size optimization?)

If solid-start externalizes every dependency in ssr, that in return means, that the user cannot use devdeps for most of their deps. If they forget, and install one random dependency falsely with -D, their app would literally fail in production (with an optimized docker image).

Jason's (Preact) Microbundle solves this workflow issues with some trick: it just externalizes all pkg.dependencies and pkg.peerDependencies, which also makes a lot of sense 🙂:
https://github.com/developit/microbundle/blob/9a4e2b2096d3824661738edb142b4658cf3d9d0b/src/index.js#L364

[nksaraf]

Yeah we need to do something about the externalizing logic for two reasons:

1. Make it consistent across dev/prod between vite and rollup as mentioned by @katywings
2. Automatically mark solid libraries which ship with JSX as external so users don't need to do it and face the issues of getting a bad transpilation (SSR issue with solidjs package built with "rollup-preset-solid" #95)

We could probably use the logic written by @katywings for 1, and use the solid field in package.json exports to detect if that have a JSX output for a dependency and mark that as external.

[katywings]

@nksaraf Thank you for your feedback :)

[ryansolid]

While there are still issues here related to mono-repos, and linked libraries. I think the main issue here is done we both:

1. pass through external to ssr
2. use https://github.com/svitejs/vitefu to identify solid packages and not have them get bad transpilation. We also fallback to ESBuild Solid plugin.

Am I missing anything?

[katywings]

@ryansolid "pass through external to ssr" thats the important one 💪, tried it out with the redis example and it works ❤️.

The ugly thing is that pkg.dependencies still have to be manually externalized ;). I still would prefer a solution like https://github.com/developit/microbundle/blob/9a4e2b2096d3824661738edb142b4658cf3d9d0b/src/index.js#L364 as I think its a better "standard" approach for most use cases, to directly use dependencies from node_modules, if they are available. Then if people don't want to externalize certain dependencies they can move them to devDependencies.

[katywings]

@ryansolid Another quick info on this: maybe we should track this on a separate issue, but there is a special detail with dynamic imports. E.g if I have a dynamic import like this: import("vanilla-colorful/hex-alpha-color-picker.js"); I actually have to very explicitly externalize this with external: ['vanilla-colorful/hex-alpha-color-picker.js'].

[ryansolid]

The ugly thing is that pkg.dependencies still have to be manually externalized ;). I still would prefer a solution like https://github.com/developit/microbundle/blob/9a4e2b2096d3824661738edb142b4658cf3d9d0b/src/index.js#L364 as I think its a better "standard" approach for most use cases, to directly use dependencies from node_modules, if they are available. Then if people don't want to externalize certain dependencies they can move them to devDependencies.

That's an interesting suggestion. I'm quite frustrated with this all in general. Svelte's Vitefu helps with Solid packages but having to worry about like random server libraries is pretty painful. I need more opinions cross checks to see if others are handling things this way.

[ryansolid]

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.