-
Notifications
You must be signed in to change notification settings - Fork 409
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add three build profiles and infrastructure for their toml config #440
Conversation
} | ||
BuildProfile::Dev => { | ||
// Plain cargo builds use the dev cargo profile, which includes | ||
// debug info by default. |
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 reminds me that we really should add --dev
to Cargo...
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.
Yep, that would be nice.
baa523c
to
3eb5713
Compare
One of the new tests added depends on this: rustwasm/wasm-bindgen#1020 |
3eb5713
to
b73bfd1
Compare
This is ready for another round of review. |
b73bfd1
to
f3e4d62
Compare
74d1c72
to
a9f1ce8
Compare
Ok, I am pretty happy with serde and defaults stuff now. Final round of review? |
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 I wasn't clear earlier when you said you wanted to hide the unwraps, but just to confirm you didn't want to take the strategy where there's one struct that has a bunch of non-Option
fields and one that has Option
and the former implement deserialize in terms of the latter?
Also just to make sure I don't forget, we should file an issue afterwards to handle unknown keys to issue warnings on things like typos. Cargo uses https://crates.io/crates/serde_ignored and wasm-pack can probably use it as well!
|
||
/// Get this profile's configured `[wasm-bindgen.debug-js-glue]` value. | ||
pub fn wasm_bindgen_debug_js_glue(&self) -> bool { | ||
self.wasm_bindgen.debug_js_glue.unwrap() |
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.
Oh I thought you were leaning towards a strategy that removes these unwraps?
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 leaned back towards them because making the two-struct approach work with nested structs involves a lot of boilerplate and it isn't totally clear how we would know which defaults to use in the nested struct deserialization where we no longer know which profile we are deserializing for.
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! Just wanted to confirm. We can always tweak over time if necessary!
Filed #443 |
a9f1ce8
to
64d52d8
Compare
Fixes #153
Fixes #160