Skip to content

[feat] install adapters on demand #7462

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.

Sign up for GitHub

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

Merged
Rich-Harris merged 16 commits into from
Nov 15, 2022
Merged

[feat] install adapters on demand #7462

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
dd410e4
[feat] install adapters on demand
dummdidumm Nov 1, 2022
3e1dffc
oops
dummdidumm Nov 1, 2022
76fd805
lock file, more logs
dummdidumm Nov 2, 2022
8beb30a
use postinstall hook
dummdidumm Nov 8, 2022
3f5e992
maybe
dummdidumm Nov 14, 2022
b832a84
ugh
dummdidumm Nov 14, 2022
28e8576
ignore errors that are not about module not found
dummdidumm Nov 14, 2022
faadc0e
debug
dummdidumm Nov 14, 2022
7a1e06d
Merge branch 'master' into adapter-auto-on-demand
Rich-Harris Nov 14, 2022
123124a
test node_env findings
dummdidumm Nov 15, 2022
1aeaaa6
remove dev omit and set node_env through JS
dummdidumm Nov 15, 2022
c1afb75
no env whatsoever?
dummdidumm Nov 15, 2022
5418d8b
Merge branch 'adapter-auto-on-demand' of github.com:sveltejs/kit into…
Rich-Harris Nov 15, 2022
5449fd2
this seems to work?
Rich-Harris Nov 15, 2022
bdaf16f
diy
Rich-Harris Nov 15, 2022
40ac2af
tweak copy to mention deployment configuration
Rich-Harris Nov 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/orange-geckos-bake.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/adapter-auto': patch
---

[feat] install adapters on demand
63 changes: 45 additions & 18 deletions packages/adapter-auto/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
import { execSync } from 'child_process';
import { dirname } from 'path';
import { fileURLToPath } from 'url';
import { adapters } from './adapters.js';

/** @type {import('./index').default} */
Expand All @@ -10,31 +13,55 @@ for (const candidate of adapters) {

try {
module = await import(candidate.module);
Copy link
Member

@Rich-Harris Rich-Harris Nov 1, 2022

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@dummdidumm dummdidumm Nov 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

@Rich-Harris Rich-Harris Nov 1, 2022

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

@dummdidumm dummdidumm Nov 2, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Member

@Rich-Harris Rich-Harris Nov 2, 2022

Choose a reason for hiding this comment

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

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


fn = () => {
const adapter = module.default();
return {
...adapter,
adapt: (builder) => {
builder.log.info(`Detected environment: ${candidate.name}. Using ${candidate.module}`);
return adapter.adapt(builder);
}
};
};

break;
} catch (error) {
if (
error.code === 'ERR_MODULE_NOT_FOUND' &&
error.message.startsWith(`Cannot find package '${candidate.module}'`)
) {
throw new Error(
`It looks like ${candidate.module} is not installed. Please install it and try building your project again.`
);
try {
console.log(`Installing ${candidate.module} on the fly...`);
execSync(
`${process.platform === 'win32' ? 'set' : ''} NODE_ENV=ignore_me && npm install ${
candidate.module
} --no-save --omit=dev --no-package-lock`,
{
stdio: 'inherit',
cwd: dirname(fileURLToPath(import.meta.url))
}
);
Copy link
Member

@Conduitry Conduitry Nov 15, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@dummdidumm dummdidumm Nov 15, 2022

Choose a reason for hiding this comment

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

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

Copy link
Member

@Conduitry Conduitry Nov 15, 2022

Choose a reason for hiding this comment

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

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.

module = await import(candidate.module);
console.log(
`Successfully installed ${candidate.module} on the fly. If you plan on staying on this deployment platform, consider switching out @sveltejs/adapter-auto for ${candidate.module} for faster and more robust installs.`
);
} catch (e) {
// if (
// error.code === 'ERR_MODULE_NOT_FOUND' &&
// error.message.startsWith(`Cannot find package '${candidate.module}'`)
// ) {
throw new Error(
`Could not install ${candidate.module} on the fly. Please install it yourself by adding it to your package.json's devDependencies and try building your project again.`
);
// }
// ignore other errors, but print them
console.warn(e);
}
} else {
throw error;
}

throw error;
}

fn = () => {
const adapter = module.default();
return {
...adapter,
adapt: (builder) => {
builder.log.info(`Detected environment: ${candidate.name}. Using ${candidate.module}`);
return adapter.adapt(builder);
}
};
};

break;
}
}

Expand Down
5 changes: 0 additions & 5 deletions packages/adapter-auto/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,6 @@
"format": "pnpm lint --write",
"check": "tsc"
},
"dependencies": {
"@sveltejs/adapter-cloudflare": "workspace:*",
"@sveltejs/adapter-netlify": "workspace:*",
"@sveltejs/adapter-vercel": "workspace:*"
},
"devDependencies": {
"@types/node": "^16.11.68",
"typescript": "^4.8.4"
Expand Down
7 changes: 0 additions & 7 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.