-
Notifications
You must be signed in to change notification settings - Fork 337
Conversation
This looks pretty good to me, except could we move this out of the |
This change adds proper construction of the worker metadata, previously it was an error-prone string. `bindings.rs` is used to build certain binding type, currently only wasm_module is supported.
8ca066e
to
3d03916
Compare
Good point @cwndrws, I moved the metadata and bindings into settings. |
let mut metadata_file = File::create(self.metadata_path())?; | ||
metadata_file.write_all(create_metadata(self).as_bytes())?; | ||
metadata_file.write_all(metadata.as_bytes())?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this didn't originate from this PR, but why do we write the metadata to a file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its useful if you want to use wrangler as a "build-only" tool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But metadata isn't used unless you're publishing to Cloudflare, so we support building with wrangler and then publishing with curl or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe as part of a CI process where the CI handles upload because wrangler doesn't support complex config for handling staging environments etc. I think it's useful for all "files" that we upload for multipart to end up on disk somewhere for this reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it stay on disk? or is it cleaned up later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was some back and forth with the metadata file lately, see #276. The file is store in ./worker as part of the worker "bundle", it is persistent.
I'm sure some people will commit their worker, it make sense to have a consistent state including the metadata, it's useful for the preview and debugging too.
probably not now, but settings is ripe for some extractions. |
.to_string() | ||
bindings.push(Binding::new_wasm_module( | ||
bundle.get_wasm_binding(), | ||
bundle.get_wasm_binding(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once is the name and the other the multipart part.
I don't feel strong about the directory, feel free to suggest something else. |
I think it's just fine for now, it's sort of a "needs design" thing that will be relatively painless to refactor after the fact. |
} | ||
|
||
impl Binding { | ||
pub fn new_wasm_module(name: String, part: String) -> Binding { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems odd; why would we want a wasm-specific method on the binding trait? or am i reading this wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is too add for each binding type a method (like a factory, constructor), see https://github.com/cloudflare/wrangler/pull/215/files#diff-deeb786858380f08f8f4d40680620a9a if it makes more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok that makes sense, thank you.
I think bundle deserves its own module, so there is a home within the code for "things that wrangler writes out at the end of a build" |
let's land . this . after an approving review from @ashleymichal |
This change adds proper construction of the worker metadata, previously
it was an error-prone string.
bindings.rs
is used to build certain binding type, currently onlywasm_module is supported.