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

adapter-auto: defer adapter installation until required #7739

Merged
merged 2 commits into from Nov 21, 2022

Conversation

gtm-nayan
Copy link
Contributor

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
Copy link

changeset-bot bot commented Nov 21, 2022

🦋 Changeset detected

Latest commit: 84fa75a

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

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me and should probably resolve the "double package" problem one can run into - leaving this open so others can give their opinions on this

@Rich-Harris
Copy link
Member

If we add new capabilities to adapters in future — for example, if they're allowed to control aspects of the dev environment — then this won't work

@Conduitry
Copy link
Member

Probably we don't want the auto adapter to be affecting dev mode? It wouldn't have any of the env vars it looks at anyway. Presumably people should explicitly install their target adapter if they want to have it impact dev mode too.

@gtm-nayan
Copy link
Contributor Author

To provide platform specific features in dev mode you'd have to know what platform you'd be deploying to, and at that point adapter-auto can be swapped out manually.

I thought the idea was that adapter-auto would serve as a minimal deployable setup for new users unfamiliar with adapters and users requiring more features can use specific ones for their platform. There's a warning in adapter-auto which reflects that sentiment: If you plan on staying on this deployment platform, consider replacing @sveltejs/adapter-auto with ${match.module}. This will give you faster and more robust installs, and more control over deployment configuration.

@Rich-Harris
Copy link
Member

That's a fair point! I rescind my earlier comments. That said, I'm not 100% sure I understand what problem this solves?

/**
* @returns {Promise<Adapter | undefined>} The corresponding adapter for the current environment if found otherwise undefined
*/
async function get_adapter() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd make it clearer from the name that this is installing the adapter as get sounds side-effect free

Suggested change
async function get_adapter() {
async function install_adapter() {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In some cases it won't install anything though. We could make it get_or_install_adapter or whatever but i think it's clear enough from context

@benmccann
Copy link
Member

I'm not 100% sure I understand what problem this solves?

Right now it's installing the adapter up-front, which seems to be doing the install in the middle of the Vite bundling causing two versions of SvelteKit to end up in the final bundle. By waiting until the bundle is written, hopefully that's fixed.

I'm not 100% sure how to test this other than merging it and updating the sites repo. There's probably a way, but I'd be just as happy to merge this even if it doesn't work as I think it's a nice way to structure the code, so I'd say we merge this and see if it works

@Rich-Harris Rich-Harris merged commit 536c40c into sveltejs:master Nov 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants