Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

Use serde for metadata #285

Merged
merged 2 commits into from
Jul 1, 2019
Merged

Use serde for metadata #285

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d03916
refactor: use serde for metadata
xtuc Jul 1, 2019
c5ec13e
Merge branch 'master' into refactor-use-metadata-struct2
xtuc Jul 1, 2019
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
60 changes: 18 additions & 42 deletions src/commands/build/wranglerjs/bundle.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ use std::path::{Path, PathBuf};
use log::info;

use crate::commands::build::wranglerjs::output::WranglerjsOutput;
use crate::settings::binding::Binding;
use crate::settings::metadata;
use crate::terminal::message;

// Directory where we should write the {Bundle}. It represents the built
Expand Down Expand Up @@ -50,8 +52,9 @@ impl Bundle {

script_file.write_all(script.as_bytes())?;

let metadata = create_metadata(self).expect("could not create metadata");
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())?;
Copy link

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?

Copy link
Contributor

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

Copy link

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?

Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Member Author

@xtuc xtuc Jul 1, 2019 •
edited
Loading

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.


// cleanup {Webpack} dist, if specified.
if let Some(dist_to_clean) = &wranglerjs_output.dist_to_clean {
Expand Down Expand Up @@ -109,31 +112,20 @@ pub fn create_prologue() -> String {
}

// This metadata describe the bindings on the Worker.
fn create_metadata(bundle: &Bundle) -> String {
info!("create metadata; wasm={}", bundle.has_wasm());
fn create_metadata(bundle: &Bundle) -> Result<String, serde_json::error::Error> {
let mut bindings = vec![];

if bundle.has_wasm() {
format!(
r#"
{{
"body_part": "script",
"bindings": [{{
"name": "{name}",
"type": "wasm_module",
"part": "{name}"
}}]
}}
"#,
name = bundle.get_wasm_binding(),
)
.to_string()
} else {
r#"
{
"body_part": "script"
}
"#
.to_string()
bindings.push(Binding::new_wasm_module(
bundle.get_wasm_binding(),
bundle.get_wasm_binding(),
Copy link
Contributor

Choose a reason for hiding this comment

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

twice?

Copy link
Member Author

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.

));
}

serde_json::to_string(&metadata::Metadata {
body_part: "script".to_string(),
bindings,
})
}

#[cfg(test)]
Expand Down Expand Up @@ -167,14 +159,7 @@ mod tests {
let contents =
fs::read_to_string(&bundle.metadata_path()).expect("could not read metadata");

assert_eq!(
contents,
r#"
{
"body_part": "script"
}
"#
);
assert_eq!(contents, r#"{"body_part":"script","bindings":[]}"#);

cleanup(out);
}
Expand Down Expand Up @@ -233,16 +218,7 @@ mod tests {

assert_eq!(
contents,
r#"
{
"body_part": "script",
"bindings": [{
"name": "wasmprogram",
"type": "wasm_module",
"part": "wasmprogram"
}]
}
"#
r#"{"body_part":"script","bindings":[{"type":"wasm_module","name":"wasmprogram","part":"wasmprogram"}]}"#
);

cleanup(out);
Expand Down
14 changes: 14 additions & 0 deletions src/settings/binding.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
use serde::Serialize;

#[derive(Serialize, Debug)]
#[serde(tag = "type")]
pub enum Binding {
#[allow(non_camel_case_types)]
wasm_module { name: String, part: String },
}

impl Binding {
pub fn new_wasm_module(name: String, part: String) -> Binding {
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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.

Binding::wasm_module { name, part }
}
}
9 changes: 9 additions & 0 deletions src/settings/metadata.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
use serde::Serialize;

use crate::settings::binding::Binding;

#[derive(Serialize, Debug)]
pub struct Metadata {
pub body_part: String,
pub bindings: Vec<Binding>,
}
2 changes: 2 additions & 0 deletions src/settings/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,4 @@
pub mod binding;
pub mod global_user;
pub mod metadata;
pub mod project;