Skip to content

fix(toolchain): mark external deps #2713

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

Closed
NathanFlurry wants to merge 1 commit into from
Closed

fix(toolchain): mark external deps #2713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
83 changes: 46 additions & 37 deletions packages/toolchain/cli/src/util/deploy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ pub async fn deploy(opts: DeployOpts<'_>) -> Result<Vec<Uuid>> {

// Setup Rivetkit
let mut _rivet_actor_tempfiles = Vec::new();
let mut _rivet_actor_tempdirs = Vec::new();
// let mut _rivet_actor_tempdirs = Vec::new();
if let Some(rivetkit) = &root.rivetkit {
// Create service token
let service_token =
Expand Down Expand Up @@ -175,37 +175,42 @@ pub async fn deploy(opts: DeployOpts<'_>) -> Result<Vec<Uuid>> {
.await?;

// Determine server runtime
let function_build = if rivetkit.build.image.is_some()
|| rivetkit.build.dockerfile.is_some()
{
// Use user-provided Docker config
rivetkit.build.clone()
} else {
// Auto-generate server Dockerfile
//
// Has to be in the project path in order to use NPM dependencies
let dockerfile_tempdir = tempfile::Builder::new().prefix("rivet-server-").tempdir()?;
let dockerfile_path = dockerfile_tempdir.path().join("Dockerfile");
let dockerignore_path = dockerfile_tempdir.path().join("Dockerfile.dockerignore");
_rivet_actor_tempdirs.push(dockerfile_tempdir);

let server_path = rivetkit.server.clone();
tokio::fs::write(
&dockerfile_path,
generate_server_dockerfile(&project_root, server_path),
)
.await?;
tokio::fs::write(&dockerignore_path, generate_server_dockerignore()).await?;

toolchain::config::build::docker::Build {
dockerfile: Some(dockerfile_path.display().to_string()),
build_path: Some(project_root.display().to_string()),
image: None,
build_target: None,
build_args: None,
unstable: rivetkit.build.unstable.clone(),
}
};
let function_build =
if rivetkit.build.image.is_some() || rivetkit.build.dockerfile.is_some() {
// Use user-provided Docker config
rivetkit.build.clone()
} else {
// Auto-generate server Dockerfile
//
// Has to be in the project path in order to use NPM dependencies
//
// Preserve the paths because we want to be able to let the user to test the Dockerfile
// that's printed out
let dockerfile_tempdir = tempfile::Builder::new()
.prefix("rivet-server-")
.tempdir()?
.into_path();
let dockerfile_path = dockerfile_tempdir.join("Dockerfile");
let dockerignore_path = dockerfile_tempdir.join("Dockerfile.dockerignore");
// _rivet_actor_tempdirs.push(dockerfile_tempdir);

let server_path = rivetkit.server.clone();
tokio::fs::write(
&dockerfile_path,
generate_server_dockerfile(&project_root, server_path),
)
.await?;
tokio::fs::write(&dockerignore_path, generate_server_dockerignore()).await?;

toolchain::config::build::docker::Build {
dockerfile: Some(dockerfile_path.display().to_string()),
build_path: Some(project_root.display().to_string()),
image: None,
build_target: None,
build_args: None,
unstable: rivetkit.build.unstable.clone(),
}
};

// Add function
root.functions.insert(
Expand Down Expand Up @@ -716,8 +721,6 @@ fn generate_dockerfile_for_package_manager(
package_manager: &PackageManager,
server_path_js: &str,
) -> String {
let copy_files = package_manager.copy_files.join(" ");

// Determine package manager specific configurations
let (base_image, setup_commands, build_cmd, runtime_cmd, add_deps_cmd) = match package_manager.name.as_str() {
"yarn" => (
Expand All @@ -732,7 +735,7 @@ fn generate_dockerfile_for_package_manager(
"",
"bunx tsc --outDir dist/ --rootDir ./",
"bun run",
"bun add @hono/node-server @hono/node-ws"
"echo noop"
),
"pnpm" => (
"node:22-alpine",
Expand All @@ -753,6 +756,7 @@ fn generate_dockerfile_for_package_manager(
let mut dockerfile = String::new();

// Builder stage
let copy_files = package_manager.copy_files.join(" ");
dockerfile.push_str(&format!("FROM {} AS builder\n\n", base_image));
dockerfile.push_str("WORKDIR /app\n\n");
dockerfile.push_str(&format!("# Copy package files\nCOPY {} ./\n\n", copy_files));
Expand Down Expand Up @@ -793,9 +797,14 @@ fn generate_dockerfile_for_package_manager(
dockerfile.push_str("\n\n");
}

let app_copy_files = package_manager
.copy_files
.iter()
.map(|x| format!("/app/{x}"))
.collect::<Vec<_>>()
.join(" ");
dockerfile.push_str(&format!(
"COPY --from=builder --chown=rivet:rivet /app/{} ./\n\n",
copy_files
"COPY --from=builder --chown=rivet:rivet {app_copy_files} ./\n\n"
));

// Install production dependencies
Expand Down
7 changes: 7 additions & 0 deletions packages/toolchain/js-utils-embed/js/src/tasks/build/build.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ export async function build(input: Input): Promise<Output> {
stdLibBrowser[ `node:${packageName}` ] = packagePath;
}

console.log('foo')
Copy link
Copy Markdown
Contributor

@graphite-app graphite-app Bot Jul 3, 2025

Choose a reason for hiding this comment

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

This debug console.log('foo') statement should be removed before merging. It appears to be unintentional debugging code that made its way into the PR.

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.


const result = await esbuild.build({
absWorkingDir: input.projectRoot,
entryPoints: [input.entryPoint],
Expand All @@ -67,6 +69,11 @@ export async function build(input: Input): Promise<Output> {
inject: [stdLibInject],
plugins: [stdLibPlugin(stdLibBrowser)],
external: [
"node:*",
"node:fs",
"node:fs/promises",
"fs",
"fs/promises",
// Wasm must be loaded as a separate file manually, cannot be bundled
"*.wasm",
"*.wasm?module",
Expand Down
7 changes: 6 additions & 1 deletion packages/toolchain/js-utils-embed/js/src/tasks/build/preset.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ const nodeCompatModules = [
//"url",
//"util/types",
//"zlib",
"fs",
"fs/promises",
];

// Modules implemented via a mix of workerd APIs and polyfills.
Expand All @@ -48,6 +50,9 @@ const hybridNodeCompatModules = [
"util",
];

const externalModules = nodeCompatModules.flatMap((p) => [p, `node:${p}`])
Copy link
Copy Markdown
Contributor

@graphite-app graphite-app Bot Jul 3, 2025

Choose a reason for hiding this comment

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

The externalModules variable is defined but unused, while the same expression is duplicated in the externa property below. Consider either:

  1. Using the variable in the property: externa: externalModules
  2. Removing the unused variable definition

Also note that the property name appears to have a typo - externa should likely be external based on the context.

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

console.log('modules', externalModules);
Copy link
Copy Markdown
Contributor

@graphite-app graphite-app Bot Jul 3, 2025

Choose a reason for hiding this comment

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

This debug console.log statement should be removed before merging. It appears to be printing the external modules list, which is likely only needed during development.

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.


export const rivetPreset: Preset = {
meta: {
name: "unenv:rivet",
Expand Down Expand Up @@ -86,5 +91,5 @@ export const rivetPreset: Preset = {
//process: "@cloudflare/unenv-preset/runtime/node/process/index",
},
polyfill: [],
external: nodeCompatModules.flatMap((p) => [p, `node:${p}`]),
externa: nodeCompatModules.flatMap((p) => [p, `node:${p}`]),
Copy link
Copy Markdown
Contributor

@graphite-app graphite-app Bot Jul 3, 2025

Choose a reason for hiding this comment

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

There appears to be a typo in the property name: externa should be external. This will prevent the external modules configuration from working correctly, as the system won't recognize the misspelled property.

Suggested change
externa: nodeCompatModules.flatMap((p) => [p, `node:${p}`]),
external: nodeCompatModules.flatMap((p) => [p, `node:${p}`]),

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

};
Loading