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

Don't break wrangler preview when kv namespaces present #353

Merged
merged 3 commits into from
Jul 26, 2019
Merged

Don't break wrangler preview when kv namespaces present #353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3b75b51
Warn on preview if KV Namespaces present
ashleymichal Jul 23, 2019
c71ccbe
Don't send kv namespace bindings to preview
ashleymichal Jul 23, 2019
37538fd
Refactor bool arg to two public functions
ashleymichal Jul 24, 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
2 changes: 1 addition & 1 deletion src/commands/publish/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ mod upload_form;

use package::Package;
use route::Route;
use upload_form::build_script_upload_form;
use upload_form::{build_script_upload_form, build_script_upload_form_no_kv};

use log::info;

Expand Down
9 changes: 8 additions & 1 deletion src/commands/publish/preview/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ pub fn preview(

commands::build(&project)?;

let script_upload_form = publish::build_script_upload_form(project)?;
// we want to include kv namespaces when we publish, but not when we preview
let script_upload_form = publish::build_script_upload_form_no_kv(project)?;

let res = client
.post(create_address)
Expand Down Expand Up @@ -60,6 +61,12 @@ pub fn preview(
let msg = format!("Your worker responded with: {}", worker_res);
message::preview(&msg);

if project.kv_namespaces.is_some() {
message::warn(
"KV Namespaces are not supported in the preview. Consider defining a fallback value.",
);
}

open(preview_host, https, script_id)?;

Ok(())
Expand Down
13 changes: 12 additions & 1 deletion src/commands/publish/upload_form/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,19 @@ use wasm_module::WasmModule;
use super::{krate, Package};

pub fn build_script_upload_form(project: &Project) -> Result<Form, failure::Error> {
build_script_upload_form_with_kv(project, project.kv_namespaces())
}

pub fn build_script_upload_form_no_kv(project: &Project) -> Result<Form, failure::Error> {
build_script_upload_form_with_kv(project, Vec::new())
}

fn build_script_upload_form_with_kv(
project: &Project,
kv_namespaces: Vec<kv_namespace::KvNamespace>,
) -> Result<Form, failure::Error> {
Copy link
Contributor

@steveklabnik steveklabnik Jul 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

so, boolean parameters are generally considered to be a bad style. I actually really like what you did above, with

    let include_kv = false;

rather than just passing false and true. But we can do even better!

// note this isn't pub!
fn build_script_upload_form_with_namespaces(
    project: &Project,
    kv_namespaces: Vec<NamespaceTypeOrWhatever>,
) -> Result<Form, failure::Error> {
   // previous body of build_script_upload_form goes here
   //
   // except you don't need to set up the kv_namespaces variable
}


pub fn build_script_upload_form(
    project: &Project,
) -> Result<Form, failure::Error> {
    build_script_upload_form_with_namespaces(project, Vec::new())
}

pub fn build_script_upload_form_with_kv(
    project: &Project,
) -> Result<Form, failure::Error> {
     build_script_upload_form_with_namespaces(project, project.kv_namespaces())
}

I do not know how @ashleygwilliams feels about this kind of refactoring, but I generally prefer it. That being said, boolean parameters are not the worst, so if she prefers this code as-is, I don't think it's the end of the world.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the boolean parameter did indeed feel Gross. I like this better, ty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

implemented this, except called out the "weird" case which is building without kv.

let project_type = &project.project_type;
let kv_namespaces = project.kv_namespaces();

match project_type {
ProjectType::Rust => {
info!("Rust project detected. Publishing...");
Expand Down
4 changes: 4 additions & 0 deletions src/terminal/message.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,7 @@ pub fn working(msg: &str) {
pub fn preview(msg: &str) {
message(emoji::WORKER, msg);
}

pub fn warn(msg: &str) {
message(emoji::WARN, msg);
}