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

Provide ability to bundle dev dependencies for Node adapter #3176

Closed
Conduitry opened this issue Jan 2, 2022 · 21 comments · Fixed by #6372
Closed

Provide ability to bundle dev dependencies for Node adapter #3176

Conduitry opened this issue Jan 2, 2022 · 21 comments · Fixed by #6372
Labels
breaking change feature request New feature or request p1-important SvelteKit cannot be used by a large number of people, basic functionality is missing, etc. pkg:adapter-node
Milestone

Comments

@Conduitry
Copy link
Member

Describe the problem

Currently, the Node adapter will not bundle any external server side dependencies. This means that you need to make all of the dependencies you might need at runtime be prod dependencies. This differs from its (recent) previous behavior, and from Sapper's behavior, and from (I would argue) expected behavior.

Until now, it's been standard operating procedure to bundle as many of the server-side dependencies as possible, to reduce the deployed app size, parse time, and startup time. If there's some good reason to stop doing this, it should certainly at least be documented.

This issue may also well apply to other adapters (other than the static one), but I haven't tried to validate that.

Describe the proposed solution

I'm not entirely sure. It would be nice to be able to provide sensible default Vite configuration so that we have Vite do it, rather than do this in a separate esbuild step afterwards (either automatically or manually).

I don't know whether some different behavior from Vite would make this easier on our end, or whether this is something that Vite ought to change.

Below, I describe what I'm currently doing in userland to deal with this, but I'm not positive that this would have an equivalent in-framework analogue. (Do we have convenient access to the user's project's package.json?)

Alternatives considered

Currently, I have a vite.ssr.noExternal setting of process.env.NODE_ENV === "development" ? undefined : Object.keys(pkg.devDependencies) (where pkg is the parsed JSON from package.json) to force Vite to bundle all of my dev dependencies.

The check for development vs prod mode is there because I was having an issue using Object.keys(pkg.devDependencies) in dev mode that I haven't fully tracked down or tried to reproduce in a minimal repo, but which seem to stem from having a CJS-only dependency that Vite doesn't try to convert to ESM during dev mode.

Importance

would make my life easier

Additional Information

No response

@Karlinator
Copy link
Contributor

So there used to be an esbuild step in adapter-node, which would bundle everything. It was removed in #2931.

@Rich-Harris was this an unintended side-effect of that? I can't see it mentioned in the PR or discussion.

@rpf3
Copy link

rpf3 commented Mar 6, 2022

I ran into a similar issue to @Conduitry since upgrading and taking inspiration from this thread I added my package dependencies manually to the vite.ssr.noExternal config and got things working again. This definitely feels like a breaking change in how the Node adapter previously worked though.

A path forward would be a welcome discussion.

EDIT:

This was actually an issue with my build pipeline. We were copying the output directory of the Node adapter into a Docker image thinking it would provide a fully working application. I think what was going on is because certain packages were being externalized I needed to also copy the node_modules folder into the image.

@SomaticIT
Copy link
Contributor

Hello,

I agree with @Conduitry that it could be interesting to have an option to bundle dev dependencies when using node adapter. The workaround is working well but an option would be simpler to use.

I think it should not create a single entry file (like before) but bundles only dev dependencies as suggested by Conduitry.
An option directly on svelte-kit that apply the above workaround may also be ok.

@Rich-Harris Rich-Harris added this to the 1.0 milestone May 12, 2022
silentworks added a commit to silentworks/waiting-list that referenced this issue Jun 4, 2022
Waiting for this issue to be fixed sveltejs/kit#3176
@Conduitry
Copy link
Member Author

Conduitry commented Jul 6, 2022

As of the latest versions of SvelteKit - with a separate vite.config.js - I now need to use conditional Vite config (https://vitejs.dev/config/#conditional-config) and specify an ssr.noExternal value of command === 'build' ? Object.keys(pkg.devDependencies) : undefined.

@benmccann
Copy link
Member

benmccann commented Aug 9, 2022

Doing this makes the system harder to understand in my opinion because it makes dev different than prod (though perhaps that ship has sailed) and SvelteKit different from Vite. It also adds another way in which adapter-node is different from the rest (though the others are also different because they use esbuild).

If the adapters were Vite plugins (or could provide Vite plugins) then we could at least make this functionality available with a single option like bundleDependencies: boolean.

@loxs
Copy link

loxs commented Aug 11, 2022

Yeah, this is really annoying. Currently my docker image is 1.5G and could be 150-200M if everything was bundled

@sidvishnoi
Copy link

FWIW, we presently use a postbuild step using esbuild. This reduces our docker image size from 1GB to 20MB.

// File: scripts/postbuild.js
// In package.json: scripts.postbuild = "node scripts/postbuild.js"
require('esbuild').buildSync({
  entryPoints: ['./build/index.js'],
  outfile: './build/index.bundle.js',
  format: 'esm',
  target: 'esnext',
  platform: 'node',
  // When outputting ESM, any require calls will be replaced with the __require
  // shim, since require will not be available. However, since external paths
  // will not be bundled, they will generate a runtime error. This matters only
  // for built-in node modules.
  // https://github.com/evanw/esbuild/pull/2067#issuecomment-1152399288
  banner: {
    js: `
    import { createRequire } from 'module';
    import { fileURLToPath as urlESMPluginFileURLToPath } from "url";
    import { dirname as pathESMPluginDirname} from "path";
    const __filename = urlESMPluginFileURLToPath(import.meta.url);
    const __dirname = pathESMPluginDirname(urlESMPluginFileURLToPath(import.meta.url));
    const require = createRequire(import.meta.url);
    `,
  },
  bundle: true,
});

@Conduitry
Copy link
Member Author

Conduitry commented Aug 16, 2022

With the now-latest version of SvelteKit, @sveltejs/kit itself now has prod dependencies that need to be bundled at app build time - it used to be that these dependencies were bundled in @sveltejs/kit itself - so my above workaround doesn't work anymore. The same would presumably happen with other dev dependencies that have prod dependencies.

My new workaround is something that I think did not used to be possible in Vite 2 but which is now possible in Vite 3. Rather than telling Vite which dependencies to bundle, I tell it to bundle all dependencies, but exclude the ones that are prod dependencies of the app:

import { sveltekit } from '@sveltejs/kit/vite';
import { readFileSync } from 'fs';

const pkg = JSON.parse(readFileSync('package.json' ,'utf8'));

export default ({ command }) => ({
	plugins: [sveltekit()],
	ssr: command === 'build' && {
		external: Object.keys(pkg.dependencies),
		noExternal: true,
	},
});

@SylvainGarrigues
Copy link

Issue #5944 was marked as duplicate, yet I found out recent version of sveltekit and adapter-node makes the build dependent on the devalue package. Installing it solves the issue.

@kevpye-fabdata

This comment was marked as resolved.

@Conduitry

This comment was marked as resolved.

@kevpye-fabdata

This comment was marked as resolved.

@fmaclen

This comment was marked as outdated.

@moisesbites
Copy link

moisesbites commented Aug 20, 2022

Edit: As others pointed out, adding devalue, cookie and set-cookie-parser as production dependencies and running npm ci --prod fixes the issues, including the import { json } from '@sveltejs/kit' error I mentioned above.

I think the process of build need to add adapter node dependencies in production. It' very confusing if every time this dependencies was changed, we would need to start the app in production to discover that some packages is not there.

@Rich-Harris
Copy link
Member

Opened a PR that adds @Conduitry's solution to the default Vite config #6301

@benmccann
Copy link
Member

@fmaclen can you share a reproduction on the latest SvelteKit? I just tried @Conduitry's suggestion and it works fine for me

@fmaclen
Copy link

fmaclen commented Aug 26, 2022

@benmccann just tried @Conduitry's solution on my project again (with the latest version and the one I was using when I posted my comment) and both work as expected so the error was probably caused by some other issue specific to my repo 👍

@moisesbites
Copy link

moisesbites commented Aug 27, 2022

Edit: As others pointed out, adding devalue, cookie and set-cookie-parser as production dependencies and running npm ci --prod fixes the issues, including the import { json } from '@sveltejs/kit' error I mentioned above.

I think the process of build need to add adapter node dependencies in production. It' very confusing if every time this dependencies was changed, we would need to start the app in production to discover that some packages is not there.

If I install devalue, when run builded occurs:

file:///home/node/app/build/server/chunks/index.js:3
import devalue from "devalue";
       ^^^^^^^
SyntaxError: The requested module 'devalue' does not provide an export named 'default'

If I not install, the App don't work because it don't find devalue.

Would someone help me?

@vboulaye
Copy link

@moisesbites you should pin the devalue version to 2.0.1 (last version is 3.0.0 but the upgrade as not yet been done in kit)

here are the version that I used with a successful build yesterday :

    "cookie": "^0.5.0",
    "devalue": "2.0.1",
    "set-cookie-parser": "^2.5.1",

@moisesbites
Copy link

"devalue": "2.0.1",

@vboulaye Thank you!

It would be nice for the adapter to keep the libraries up to date with the latest versions, as for now the build installation has been delegated to the developer at the end. That would be less confusing, at least for now.

Rich-Harris added a commit that referenced this issue Aug 29, 2022
* bundle non-prod dependencies with esbuild - closes #3176

* changeset

* update README
@Conduitry
Copy link
Member Author

This should be good to go now in 1.0.0-next.88 of the Node adapter. When you update, you should remove any workarounds you added for this issue, including my ssr Vite config and including adding SvelteKit's runtime dependencies as direct prod dependencies of your app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change feature request New feature or request p1-important SvelteKit cannot be used by a large number of people, basic functionality is missing, etc. pkg:adapter-node
Projects
None yet