-
Notifications
You must be signed in to change notification settings - Fork 337
Conversation
64808c5
to
c505e36
Compare
Cargo.toml
Outdated
@@ -39,3 +39,4 @@ fs_extra = "1.1.0" | |||
|
|||
[features] | |||
vendored-openssl = ['openssl/vendored'] | |||
slow_tests = [] |
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.
what is this for? just curious.
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 test for rust is very slow, so I wanted to exclude it when running locally. It's enabled in the CI.
src/commands/build/mod.rs
Outdated
use std::path::PathBuf; | ||
use std::process::Command; | ||
|
||
use crate::terminal::message; | ||
|
||
fn write_kv_metadata(maybe_kv_namespaces: &Option<Vec<KvNamespace>>) -> Result<(), failure::Error> { |
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.
is there any important difference between a None
and a Vec<KvNamespace>
of length 0?
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 thing is that the vector is not present until you specify the key in the config. In the webpack backend I have a unwrap_or_else approach, feel free to indicate which one is prefered.
tests/build.rs
Outdated
|
||
#[cfg(feature = "slow_tests")] | ||
#[test] | ||
fn it_builds_with_kv_metadata_rust() { |
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.
is there also a test for it_builds_with_wasm_bindings_rust
? because reading the code it looks like this may be a good regression test to have.
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.
Yes I agree, we don't yet.
the problem with this PR is that it puts metadata generation in the build step, however you will find metadata generation in the publish step; which means that in practice actually you will write kv namespace bindings and then write over them immediately and end up with just your wasm binding. I think we need to actually allow the multipart form data step to be its own thing; perhaps each project needs to be able to handle serializing itself. |
Metadata generation is back at build time #276.
Rust and JavaScript doesn't have metadata auto generation, this PR is introducing it for KV.
I added a mention about the multipart from in #293. I don't know if it's related to this PR.
This is currently the case but there no need to, that's why #280 was made. |
TODO: keep the tests but remove the kv_namespace logic to merge |
9d5e6f5
to
939860d
Compare
939860d
to
9d4b5f1
Compare
Only adds a test, merging in feat-better-kv now. |
targeting #215