[feat] install adapters on demand

[dummdidumm]

Closes #5123

Marking as draft because I still need to test this on one of the providers, with npm/pnpm/yarn

Please don't delete this checklist! 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
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

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

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

[changeset-bot]

🦋 Changeset detected

Latest commit: 40ac2af

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

This PR includes changesets to release 1 package

Name	Type
@sveltejs/adapter-auto	Patch

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

[Rich-Harris]

TIL --no-package-lock — if that allows this to work across package managers, that'd be pretty sweet.

Should we add a message along the lines of 'you should add this to your package.json for faster build times in future?'

[Conduitry]

That'd only help people who look at the build logs, right? I wonder how many people that is.

Also, is there a reason for the echo to be part of the exec as well, instead of just a regular console.log?

If we get rid of that, can we do something with execFileSync like what we're doing here so we can completely avoid the shell's processing and escaping?

[dummdidumm]

What's the advantage to that? The current approach forwards the output to the main shell, so it could help in case something goes wrong.

[Rich-Harris]

That'd only help people who look at the build logs, right? I wonder how many people that is.

a small number, but larger than zero

What's the advantage to that? The current approach forwards the output to the main shell

We'd still pipe stderr and stdout. The advantage is that there's no caveats around candidate.module being some weird string that we need to deal with — npm package names should be fine, but for an example of the category of bugs it prevents, see https://github.com/sveltejs/kit/pull/6129/files.

Also execFileSync should be slightly faster as it doesn't involve a shell

[dummdidumm]

tried switching to it, can't switch because windows can't execute npm without the .cmd ending in this case

[Conduitry]

Can we have process.platform === 'win32' ? 'npm.cmd' : 'npm' as the first argument of execFileSync? Does that work?

[Rich-Harris]

am suddenly wondering if we should actually be importing the module from the app directory rather than the adapter-auto directory. in most cases it should Just Work, but you can imagine a situation where adapter-auto is installed in the workspace root while adapter-foo is installed inside the app

[dummdidumm]

Doesn't candidate.module just contain the name of the package, and the resolution algorithm should start at the adapter-auto directory and look for node_modules there, and if it's there, look for that module there? At least I (try to) use this fact by installing the package into the adapter-auto directory by running npm install inside its directory.

[Rich-Harris]

If the adapter is a dependency of the app (which it would be, if you'd already installed it) then surely resolution should start from there? (Until import.meta.resolve is stable, this would need import-meta-resolve)

[dummdidumm]

I feel like this overcomplicates things for no good reason. In what world are you using adapter-auto, but have installed a more specific adapter in another place? You either have them in the same place or switched to the one you actually want to use a long time ago.

[Rich-Harris]

It's not so hard to imagine a situation where shared dependencies are installed in the workspace root but someone installs a one-off dependency in a package — this very repo used to work that way, until we decided to move all dependencies into packages. I also wonder if 'dependencies of x can import all other dependencies of x' is guaranteed across all package managers now and in the future. It's a side-effect of the resolution algorithm plus the way package managers populate node_modules, but it's the sort of thing that feels changeable, the same way pnpm prevents the npm 3+ behaviour of allowing x to directly import all indirect dependencies

[dummdidumm]

The approach here seems to work when it comes to installing, but for some reason it

• says "cannot use import outside a module" when installing the adapter inside the auto-adapter's node_modules
• deletes some needed modules from node_modules when installing it in the top level modules

will continue to tinker around.

Update: Can't reproduce the first point anymore, so ... it works?

Things that are not ideal with this solution:

• uses the fact that module resolution algorithm traverses upwards, starting in the current file where the package is imported, looking for node_modules. Not sure how reliable this is
• duplicated node_modules: everything that's a dependency of the installed adapter and which is already part of the real node_modules will be installed again in the node_modules folder of adapter-auto. Most notably that' esbuild right now. Not sure if this has any unintended side effects.

Things we could add:

• right now this isn't tied to a specific version range of the adapters, like @Conduitry suggested in the issue. I'm not sure if it's worth it, adapter-auto is... well, pretty automatic, so installing the latest version would be ok with me. What we could do is scope it to a major version range, which would be easy to do manually once in a while, but even there I'm not sure what that would mean for adapter-auto - when would it itself get a new major? Everytime when some of the others get a new major?

[Rich-Harris]

😬

this might be easier said than done

[dummdidumm]

goddammit, why is this so unreliable to test?

Other options:

• modify the user's package.json and add adapter-whatever-matches, then run npm/yarn/pnpm install again
• find out why the heck npm install adapter-whatever --no-save --no-package-lock deletes a bunch of packages from node_modules (even without those flags)

[Rich-Harris]

I don't love solving problems by adding dependencies, but we could experiment with https://github.com/antfu/ni

[Conduitry]

I'm all for finding a better way to do this, but I think that can happen incrementally, and for right now, pretty much anything would be better than utterly flabbergasting dependency tree that @vercel/nft brings in.

I see that ni has no prod dependencies of its own, which is nice.

Some other options might be to npm install <adapter> in some temporary directory (maybe with a dummy package.json file?) and then move those files over into the user's node_modules folder. Although, yes, it certainly seems mysterious that the npm install we're doing here would be deleting the other dependencies in the first place.

[Rich-Harris]

I thought about the 'install in some other directory option' but I think with npm you'd end up with a flat directory containing the adapter and all its dependencies, and then you wouldn't be able to move them into the real node_modules without conflicts with existing dependencies, which is fine if the versions match but somewhat hairy otherwise

[dummdidumm]

The solution right now already install things in a separate node_modules folder, namely that inside of adapter-auto, and from Rich's comment it seems that doesn't work - so I'll give ni a shot.

[dummdidumm]

😬

this might be easier said than done

@Rich-Harris how did you test this PR? Did you use patch-package? I settled for testing it out differently; by adding the modified package in a sibling folder and pointing the package.json to it: "@sveltejs/adapter-auto": "./adapter-auto

[dummdidumm]

I'm completely lost as to why running an install command from execSync (or execFileSync, or using a third party command line package that claims to have better support in Windows) just deletes all other packages. If I just run npm install all packages are deleted - it's as if the package.json's (dev)dependencies are seen as empty. The same behavior happens when I install the packages inside the auto-adapter folder, it's just not a problem right now because there are no dependencies at the moment.

[Rich-Harris]

how did you test this PR? Did you use patch-package?

I'm just looking at the normal build logs. I didn't think there'd be any need to do anything funky since we're not making any changes to the package we're trying to lazily install

[dummdidumm]

I still don't know why using execSync (or any of the related commands) removes all other node_modules. It seems that doing the "on-the-fly"-install inside the postinstall hook works better for this. But looking at the CI on Vercel that still seems to have problems, because for some reason the prepare hook of TypeScript is executed, which wants to start a gulp script, which fails. And I'm not sure installing anything outside of the adapter-auto directory is a good idea - we simply don't know where the install command was executed. We can only guess by looking for a lock file.

[Conduitry]

We should be able to manually pass env vars with the env option https://nodejs.org/dist/latest-v19.x/docs/api/child_process.html#child_processexecsynccommand-options

If we do that, we can also avoid involving the shell at all, and we can use execFileSync here instead.

[dummdidumm]

execFileSync needs .cmd endings for the npm/pnpm/yarn command on windows last time I tested this. So we either have to prepend that on windows or keep using execSync

[Conduitry]

Yeah I saw your comment in that thread after I commented here. I responded over on that thread. Even if we can't make execFileSync work, it would be nice to avoid the manual setting of the env var using shell commands.

I'm actually not even sure that FOO=bar && do-something would call do-something with FOO set to bar. I don't think environment variables automatically inherit that way in shell scripts without an export.

[dummdidumm]

Still don't understand why running npm install inside the adapter-auto folder doesn't work. Running npm install from the root without NODE_ENV set works. Now the question is if we feel confident enough to always know where to run that command from. process.cwd() isn't necessarily the same directory from which pnpm/npm/yarn install ran.

[Conduitry]

I think the various npm/pnpm/yarn commands always put you in the same directory as the corresponding package.json file when running any of the scripts in it.

[dummdidumm]

I think so, too, but this wouldn't help you in case of a monorepo, where you run pnpm install at the root but pnpm build inside a sub directory (like the Kit monorepo). Likelyhood of this situation is probably small given that the adapters are probably run scoped to the current package, but who knows.