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

Auto adapter should download other adapters on demand #5123

Closed
Conduitry opened this issue May 31, 2022 · 3 comments · Fixed by #7462
Closed

Auto adapter should download other adapters on demand #5123

Conduitry opened this issue May 31, 2022 · 3 comments · Fixed by #7462
Labels
adapters - general Support for functionality general to all adapters feature request New feature or request
Milestone

Comments

@Conduitry
Copy link
Member

Describe the problem

Currently, adapter-auto has the other official serverless adapters as prod dependencies, so it can delegate to the correct one at runtime based on the current environment variables. This used to be an acceptable inefficiency, but the recent addition of @vercel/nft to the Vercel adapter (and thus to the auto adapter) added a lot of dependencies.

In the barest SvelteKit starter template, two thirds of the dependencies that get installed are only used by the auto adapter.

Describe the proposed solution

To make builds reproducible, each version of the auto adapter should pull in specific fixed versions of all of the other adapters, rather than pull in the latest one. Each time any other dependency has a new version cut, the auto adapter should also get a new release, which installs that new version.

It would be nice to be able to use pnpm/changesets to automate the release of the auto adapter whenever another adapter is getting released. We might be able to use peerdeps on the other adapters at version workspace:* and have this automatically handled, like workspace:* prod deps do. If we do this, we should use peerDependenciesMeta to make sure that installing the auto adapter doesn't install the other adapters, which would defeat the whole purpose here.

Alternatives considered

No response

Importance

nice to have

Additional Information

If we are only installing these dependencies on-demand and as part of the build happening in the cloud provider's remote build servers, I'm not sure whether it matters a ton whether we use the same package manager as was used to install the rest of the project's dependencies. If you run npm install something in a package that had previously used pnpm, does that remove any of the previous dependencies?

Should the selected adapter be installed as a dependency of the SvelteKit app or of the auto adapter? Since the ESM analogues of the require.resolve API aren't all in place yet, would one make our job harder when trying to delegate to the appropriate underlying adapter?

@Conduitry Conduitry added feature request New feature or request adapters - general Support for functionality general to all adapters labels May 31, 2022
@Rich-Harris
Copy link
Member

If you run npm install something in a package that had previously used pnpm, does that remove any of the previous dependencies?

I think it does get a bit chaotic. It shouldn't affect anything (whatever other modules are needed will already be in memory by that time, so it doesn't matter if they get wiped from disk) but I would expect using the same package manager that was used for everything else to result in a faster install.

Should the selected adapter be installed as a dependency of the SvelteKit app or of the auto adapter? Since the ESM analogues of the require.resolve API aren't all in place yet, would one make our job harder when trying to delegate to the appropriate underlying adapter?

The SvelteKit app — we want people to be able to (for example) install adapter-netlify as a dev dependency in their app, and for adapter-auto to pick that up. We can use https://www.npmjs.com/package/import-meta-resolve until import.meta.resolve is unflagged.

@dummdidumm
Copy link
Member

dummdidumm commented Oct 31, 2022

How reliable would this be? Are there ways you can invoke the build that fail detection of what package manger is in use or where to install to? Would people notice that the package manager lockfile is changed during build and be confused about that? Are there cases where people want to split install and build into seperate phases and installing is no longer allowed in the build phase, failing the CI build?

@Conduitry
Copy link
Member Author

I don't really think many of those would be concerns. I believe we should be able to call npm with --no-save --no-package-lock (or something like that) to have it not update any of the files, if that was the goal. Even if they are changed, I don't think that would be something people would even notice if it did happen, since the adapter would only be kicking in when the site is built in its host's remote build server (because each one looks for specific environment variables being set). And if you're using your host's provided build service that runs on their end, I doubt you'd be able to have the control to split things into different phases.

If you're building on your own servers or in your own CI for some reason, you can just use that adapter directly. The only way a lot of this stuff could come up as far as I can tell is if you're using the Node adapter, and that's currently not one of the ones that the auto adapter delegates to, and I think it should remain that way.

dummdidumm added a commit that referenced this issue Nov 1, 2022
Rich-Harris added a commit that referenced this issue Nov 15, 2022
* [feat] install adapters on demand

Closes #5123

* oops

* lock file, more logs

* use postinstall hook

* maybe

* ugh

* ignore errors that are not about module not found

* debug

* test node_env findings

* remove dev omit and set node_env through JS

* no env whatsoever?

* this seems to work?

* diy

* tweak copy to mention deployment configuration

Co-authored-by: Rich Harris <hello@rich-harris.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adapters - general Support for functionality general to all adapters feature request New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants