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

Make worker-build support custom JS shims #686

Merged

Conversation

LuisDuarte1
Copy link
Contributor

@LuisDuarte1 LuisDuarte1 commented Dec 23, 2024

This allows for users to provide their own JS shim and allowing them to customize behaviour: panic handling, wasm coredumps, and so on...

It defaults to the embedded shim in the binary to avoid breaking changes.

zebp
zebp previously approved these changes Dec 23, 2024
@zebp
Copy link
Collaborator

zebp commented Dec 23, 2024

I think in the future we should explore different bundling modes, currently we always export a valid Worker but it seems like we may want a future where we export function/class stubs for people to do however they'd like and leave making a valid worker entirely up to them without them doing a custom shim. I'm fine with supporting this until then, but I think this is indicative of the holes in worker-build.

Comment on lines 41 to 50
let custom_shim = match env::var("CUSTOM_SHIM") {
Ok(path) => {
let path = Path::new(&path).to_owned();

if !path.exists() {
None
} else {
println!("Using custom shim from {}", path.display());
Some(path)
}
}
Err(_) => None,
};

let shim_template = match custom_shim {
Some(path) => read_to_string(path)?,
None => SHIM_TEMPLATE.to_owned(),
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could these two match statements be collapsed to avoid a TOCTOU bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fair, i'll change it :)

This allows for users to provide their own JS shim and allowing them to
customize behaviour: panic handling, wasm coredumps, and so on...

It defaults to the embedded shim in the binary to avoid breaking
changes,
@LuisDuarte1 LuisDuarte1 force-pushed the feature/worker-build-custom-shim branch from bbbf9d9 to 0a31882 Compare January 6, 2025 14:16
@LuisDuarte1 LuisDuarte1 requested review from mendess and zebp January 6, 2025 14:18
@mendess
Copy link
Collaborator

mendess commented Jan 6, 2025

@zebp I'm going to merge this through CI because the error does not seem to be related at all with the proposed changes

@mendess mendess merged commit 2f401a4 into cloudflare:main Jan 6, 2025
2 of 3 checks passed
@LuisDuarte1 LuisDuarte1 deleted the feature/worker-build-custom-shim branch January 6, 2025 14:34
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.

3 participants