deno
deno copied to clipboard
denoland
Add default implementation for WorkerOptions
This adds an implementation of Default for both WorkerOptions and
BootstrapOptions. Since both of these structs are rather big, this
should make it easier for people unfamiliar with the internals to focus
on the options relevant to them.
As a user of deno_runtime I feel like these should serve as good
defaults, getting people them started without having to tweak the
runtime. Additionally even if some changes are made, the usage of
..Default::default() will significantly help with code clarity and
verbosity.
Some of these defaults might be somewhat questionable, but I think the convenience of ..Default::default() still outweighs that. However there is the option of using a builder pattern, which would allow to still have some mandatory options while the rest could be relied upon as fallbacks.
I think the majority of deno_runtime consumers would rely on the defaults of most WorkerOptions, so having some methodology that doesn't require setting every single one of them makes sense to me and would likely clean up consumer code significantly.
Interesting PR, I think we could land as is (or with some minor changes). Please make sure your code your code passes lint and format.
Personally, I'd love to see this being merged. 👍
Having no default makes creating workers relatively cumbersome. As an example, in the case of this demo, it makes up over half of the code:
https://github.com/denoland/roll-your-own-javascript-runtime/compare/main...mlafeldt:roll-your-own-javascript-runtime:deno_runtime
All defaults except for runtime_version make sense to me. I don't know how it's being used, but an empty string or "n/a" might be less confusing than a wrong version (Deno vs deno_runtime)?
Would also reduce the boilerplate in tests and examples, e.g.
- https://github.com/denoland/deno/blob/25a1cc1b28ee2f37deb6305c0feb66d4eec50804/runtime/worker.rs#L402
- https://github.com/denoland/deno/blob/25a1cc1b28ee2f37deb6305c0feb66d4eec50804/runtime/examples/hello_runtime.rs#L29
@cd-work could you please rebase this PR? I think we should land it after all
I've rebased it, I'm not sure the CI failure is related to this PR?
It's not, CI should pass now