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.

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

[feat] install adapters on demand #7462

Merged
merged 16 commits into from
Nov 15, 2022
Merged

[feat] install adapters on demand #7462

Show file tree
Hide file tree
Changes from 1 commit
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
43 changes: 27 additions & 16 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,39 @@ for (const candidate of adapters) {

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

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

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

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

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 {
execSync(
`echo "Installing ${candidate.module}" && npm install ${candidate.module} --no-save --no-package-lock`,
Copy link
Member

Choose a reason for hiding this comment

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

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?'

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

{ stdio: 'inherit', cwd: dirname(fileURLToPath(import.meta.url)) }
);
module = await import(candidate.module);
} catch (e) {
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.`
);
}
}

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