feat: unify upload form building #329

ashleymichal · 2019-07-18T17:50:28Z

This PR introduces structs that are eerily similar to #300 , with the main difference being that the bulk of the changes target the publish process, since that's where the multipart form needs to be built. Hopefully it is clear how this can be extended to add KV Namespaces as Bindings. If it is not, feedback is appreciated!

Further work could be done to make the job of building the Assets to be the responsibility of the Project struct, but this is already pretty big. The best thing happening here is that the form is being built based on the Project, (type for now), which leaves room for that info to come from project config (i.e. project.kv_namespaces).

Steps taken:

WranglerJS build step no longer writes metadata.json file
Publish step no longer looks for metadata.json file in the filesystem (another "breaking" change)
Publish step generates form data based on Project
Data structures representing the parts of a Project we want to upload (Assets and its children), and how to translate them into the different parts of a Form (Files and Bindings).
all project types call the same form building logic, "assets" are configured per project.

src/commands/publish/upload_form/mod.rs

src/commands/publish/upload_form/script.rs

src/commands/publish/upload_form/wasm_module.rs

src/commands/publish/upload_form/mod.rs

src/commands/publish/upload_form/script.rs

xortive

the To<Type> pattern in here smells of a reimplementation of https://doc.rust-lang.org/std/convert/trait.Into.html that clones for you. I don't love this pattern as it is hiding clones behind a function call, and I wonder if having a top level type to represent a resource, and a binding type that implements From<Resource> with options in the necessary places would be better. Not necessarily reimplementing pull/#313, but just taking those parts from it.

src/commands/publish/upload_form/mod.rs

src/commands/publish/upload_form/wasm_module.rs

ashleymichal · 2019-07-19T00:43:54Z

the To<Type> pattern in here smells of a reimplementation of https://doc.rust-lang.org/std/convert/trait.Into.html that clones for you. I don't love this pattern as it is hiding clones behind a function call, and I wonder if having a top level type to represent a resource, and a binding type that implements From<Resource> with options in the necessary places would be better. Not necessarily reimplementing pull/#313, but just taking those parts from it.

what's tricky about the From trait is that it consumes the first resource, but in the case of wasm modules i actually need two things: a file and a binding. perhaps the wording "ToThing" is misleading? and/or the use of traits is overkill and we could just duck type? 🤷‍♀ That is, the method WasmModule#file can be implemented without a ToFile trait, etc, which may just be better now since the trait isn't used in any other function definitions.

ashleymichal · 2019-07-23T15:02:38Z

rebased onto master. no merge conflicts.

* The preview service in particular streams the request form, and requires that the "metadata" part be set first, so this order is important. * Error out on 400+ status codes for Preview service responses to provide better error messaging.

ashleygwilliams

some nits but overall LGTM!

ashleygwilliams · 2019-07-25T15:15:33Z

src/commands/publish/upload_form/file.rs

@@ -0,0 +1,5 @@
+#[derive(Debug)]
+pub struct File {


we should evaluate what we're doing here! we should probably use a rust builtin... but the "name" here seems like it needs sorting

got this out, getting what we need from the paths for all the things.

src/commands/publish/upload_form/mod.rs

src/commands/publish/upload_form/project_assets.rs

ashleygwilliams · 2019-07-25T15:34:14Z

src/commands/publish/upload_form/mod.rs

+            if bundle.has_wasm() {
+                let wasm_module = WasmModule {
+                    path: bundle.wasm_path(),
+                    filename: bundle.get_wasm_binding(),


so let's make filename/binding the same in Webpack as RustWasm and always point to the same thing - either wasm or wasmprogram - we can file another issue for it

src/commands/publish/upload_form/mod.rs

src/commands/publish/upload_form/wasm_module.rs

* namespace wranglerjs::Bundle for grep-ability * don't clone

ashleygwilliams

ready to merge once you remove the unwraps!

ashleygwilliams · 2019-07-26T01:43:44Z

src/commands/publish/upload_form/mod.rs

+    Ok(form)
+}
+
+fn filename_from_path(path: &String) -> String {


this should return a Result<String, failure::Error> and the unwraps should be replaced with ?

turns out the unwraps were unwrapping Option types, rather than Result types. do you think this fn should give an error if it gets back a None, bc I'm inclined to feel the caller should. maybe give ProjectAssets a new method that checks the path for a non-empty filename?

I'd probably make this return Option<String> in that case, and have the caller adapt the None to its own kind of error. I think there's good arguments both ways; I'm interested to hear what @ashleygwilliams says

@steveklabnik i'm inclined to do something like this, the reasoning being that filename_from_path is kind of just a utility that i feel does not need to be opinionated about what a valid filename is, as it doesn't imply an application of the filename, whereas i could check either before constructing the ProjectAssets struct, or inside the constructor.

* return Option from filename_from_path * add constructor methods to WasmModule and ProjectAssets * add getter methods to WasmModule and ProjectAssets

ashleymichal added refactor changelog - feature labels Jul 18, 2019

ashleymichal force-pushed the alewis/feat-generate-metadata branch from a8880de to 02e550a Compare July 18, 2019 17:53