initial attempt poc at wasm #47

cheapsteak · 2020-07-11T19:30:36Z

apologies, first time reading/writing rust! XD
I referenced mdn's article to get this sort of working

probably wouldn't want to merge this though since I ripped out the feature to load external stylesheets

wasm-pack build would fail when trying to build openssl-sys that attohttpc brings in

Is there a way in rust to conditionally exclude a dependency from the main package?

Stranger6667 · 2020-07-11T20:31:15Z

Thank you very much for your contribution!

Is there a way in rust to conditionally exclude a dependency from the main package?

Yes, it is possible to control features of a dependency:

Cargo.toml

attohttpc = { version = "0", default-features = false, features = ["compress", "tls-rustls"] }

I am actually OK with switching from openssl to rustls since the latter one recently got a formal security audit. I will do this change it the master branch, and when it is done I'll post an update here, so you can rebase (there is also #48 which adds an option you mention in #45 )

Stranger6667 · 2020-07-11T20:45:16Z

#50 is merged, so some parts could be rolled back :)

Stranger6667 · 2020-07-12T12:26:23Z

wasm/src/lib.rs

+        load_remote_stylesheets: false,
+    };
+    let inliner = rust_inline::CSSInliner::new(options);
+    Ok(inliner.inline(html).unwrap())


To have a more explicit behavior on errors we can add a newtype wrapper for rust_inline::InlineError and implement From for JsValue:

struct InlineErrorWrapper(rust_inline::InlineError); impl From<InlineErrorWrapper> for JsValue { fn from(error: InlineErrorWrapper) -> Self { JsValue::from_str(error.0.to_string().as_str()) } }

Then we can use ? syntax instead of unwrap:

Ok(inliner.inline(html).map_err(InlineErrorWrapper)?)

Lets say we have an invalid @ rule specified in a "style" tag (css-inline produces an error in this case):

<html><head><style>h1 {background-color: blue;}</style></head><body><h1 style="@wrong { color: ---}">Hello world!</h1></body></html>

Then the error handling change will lead to the actual error message, that happened inside:

Uncaught (in promise) Invalid @ rule: wrong

Instead of the one below, which happens with unwrap:

Uncaught (in promise) RuntimeError: unreachable

Stranger6667 · 2020-07-12T12:28:20Z

wasm/Cargo.toml

+name = "css_inline"
+crate-type = ["cdylib"]
+
+[build-dependencies]


At the moment there is no usage of built, so I propose to remove it. In the Python bindings, it is used to provide some meta-information about the Rust side (all dependencies, environment, etc), but here I am not sure if there are any standard ways to provide such info (I'll be happy to apply them if there are)

Stranger6667 · 2020-07-12T12:31:17Z

wasm/src/lib.rs

+) -> Result<String, JsValue> {
+    let options = rust_inline::InlineOptions {
+        remove_style_tags: remove_style_tags.unwrap_or(false),
+        base_url: None,


It would be nice to provide the ability to pass all config values to InlineOptions, not sure what is the canonical way to do so in JS - is it an extra "config" object like it is done in juice? Or is it better to have multiple arguments?

will do :)
a config object would be the preferred way to go as that's the only way to provide named arguments in js

although how would something like that be written? :p

I tried

#[wasm_bindgen] pub fn inline( html: &str, options: rust_inline::InlineOptions, ) -> Result<String, JsValue> {

but understandably got

`the trait `wasm_bindgen::convert::traits::FromWasmAbi` is not implemented for `css_inline::InlineOptions<'_>`

I guess it doesn't know how to convert a struct to a javascript object?

Cool! I'll post an update regarding the struct conversion soon :) Need to do some more experiments, but the general idea could be taken from here + another newtype wrapper to avoid unwrap

To use an object for "options" argument we need to:

Use JsValue as the argument type

#[wasm_bindgen] pub fn inline(html: &str, options: JsValue) -> Result<String, JsValue> {

Convert this JsValue to rust_inline::InlineOptions<'_>
I think that the easiest way is to add an intermediate Options struct, and parse JsValue into it with serde:

#[macro_use] extern crate serde_derive; #[derive(Deserialize)] #[serde(default)] pub struct Options { inline_style_tags: bool, remove_style_tags: bool, base_url: Option<String>, load_remote_stylesheets: bool, extra_css: Option<String>, } impl Default for Options { fn default() -> Self { Options { inline_style_tags: true, remove_style_tags: false, base_url: None, load_remote_stylesheets: true, extra_css: None, } } }

Then to avoid unwrap add another newtype wrapper and From implementation for JsValue:

struct SerdeError(serde_json::Error); impl From<SerdeError> for JsValue { fn from(error: SerdeError) -> Self { JsValue::from_str(error.0.to_string().as_str()) } }

So we can parse simple JSON values like this:

#[wasm_bindgen] pub fn inline(html: &str, options: JsValue) -> Result<String, JsValue> { let options: Options = options.into_serde().map_err(SerdeError)?; // ... }

Convert this intermediate struct to InlineOptions
The conversion is fallible, so we need to implement TryFrom:

impl TryFrom<Options> for rust_inline::InlineOptions<'_> { type Error = JsValue; fn try_from(value: Options) -> Result<Self, Self::Error> { Ok(rust_inline::InlineOptions { inline_style_tags: value.inline_style_tags, remove_style_tags: value.remove_style_tags, base_url: parse_url(value.base_url)?, load_remote_stylesheets: value.load_remote_stylesheets, extra_css: value.extra_css.map(Cow::Owned), }) } }

(I used parse_url and UrlError + From implementation you added)
And the final version might look like this:

#[wasm_bindgen] pub fn inline(html: &str, options: JsValue) -> Result<String, JsValue> { let options: Options = options.into_serde().map_err(SerdeError)?; let inliner = rust_inline::CSSInliner::new(options.try_into()?); Ok(inliner.inline(html).map_err(InlineErrorWrapper)?) }

The downside is having some code duplication between the original crate options and Options, but I think it is acceptable for now since there is an extra logical step involved (those values are 1:1 mapped from JS and then URL parsing / Cow wrapping is applied). So, I think we can start like this and then maybe add some refactorings :)

that works! tested locally as well :)

there seems to be a bit of tradeoff here that the typescript bindings that end up being generated this way end up with options as any;

/ ** * @param {string} html * @param {any} options * @returns {string} */ export function inline(html: string, options: any): string;

it looks like a limitation that's being discussed/explored? rustwasm/wasm-bindgen#1591

Might need to go into the definition file to manually fiddle with the generated typescript definitions before publishing until wasm-bindgen figures this out

I tried the suggestion from https://github.com/rustwasm/wasm-bindgen/issues/1591#issuecomment-501784387 with
#[wasm_bindgen] #[derive(Deserialize)] #[serde(default)] pub struct Options { pub inline_style_tags: bool, pub remove_style_tags: bool, pub base_url: Option<String>, pub load_remote_stylesheets: bool, pub extra_css: Option<String>, }

but that fails with "the trait std::marker::Copy is not implemented for std::string::String"

Removing pub on the strings makes it sort of work

#[wasm_bindgen] #[derive(Deserialize)] #[serde(default)] pub struct Options { pub inline_style_tags: bool, pub remove_style_tags: bool, base_url: Option<String>, pub load_remote_stylesheets: bool, extra_css: Option<String>, }

this generates

export class Options { free(): void; /** * @returns {boolean} */ inline_style_tags: boolean; /** * @returns {boolean} */ load_remote_stylesheets: boolean; /** * @returns {boolean} */ remove_style_tags: boolean; }

it is still weird that a class is generated rather than an interface/dictionary; and weird that there's a free method

Hm, I need to do some research on this matter :) Would it work if we'll write proper TS types manually?

It should, although I'm not sure how to tell rust where to look for the manually written TS types?

Probably we can use some sections from here. For example skip TS generation for inline:

#[wasm_bindgen(skip_typescript)] pub fn inline(html: &str, options: &JsValue) -> Result<String, JsValue> { // ... }

And define it manually:

#[wasm_bindgen(typescript_custom_section)] const INLINE: &'static str = r#" ... some TS definitions "#;

although I'm not sure how to tell rust where to look for the manually written TS types?

Hope that it sheds some light on this if not - please, let me know if I missed something :)

Stranger6667 · 2020-07-12T12:33:35Z

One more thing - the "Verify commit message" check verifies the commit message conformance with the conventional commits specification. In this case, the commit message should be prefixed with feat:

Stranger6667 · 2020-07-12T12:35:36Z

Let me know if I can provide some more information or support here :)

Signed-off-by: Chang Wang <garnwraly@gmail.com>

Stranger6667

Looks great for me! I added a couple of small suggestions

.gitignore

wasm/src/lib.rs

wasm/Cargo.toml

Co-authored-by: Dmitry Dygalo <Stranger6667@users.noreply.github.com>

Signed-off-by: Chang Wang <garnwraly@gmail.com>

Stranger6667

Great! Thanks again for bringing this. I will create some separate issues as a follow-up (tests, CI, release, etc)

cheapsteak · 2020-07-13T13:41:35Z

Thanks so much for the patient instructions!

Stranger6667 requested changes Jul 12, 2020

View reviewed changes

cheapsteak force-pushed the chang/20/07/attempt-at-wasm branch 2 times, most recently from ece36c3 to d826571 Compare July 12, 2020 20:26

feat: wasm

a668887

Signed-off-by: Chang Wang <garnwraly@gmail.com>

cheapsteak force-pushed the chang/20/07/attempt-at-wasm branch 6 times, most recently from e69cf77 to 24de062 Compare July 12, 2020 21:43

refactor: accept InlineOptions as JsValue

6b99172

Signed-off-by: Chang Wang <garnwraly@gmail.com>

cheapsteak force-pushed the chang/20/07/attempt-at-wasm branch from 24de062 to 6b99172 Compare July 12, 2020 22:01

cheapsteak marked this pull request as ready for review July 12, 2020 22:14

Stranger6667 requested changes Jul 12, 2020

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

wasm/src/lib.rs Outdated Show resolved Hide resolved

wasm/src/lib.rs Outdated Show resolved Hide resolved

wasm/src/lib.rs Outdated Show resolved Hide resolved

wasm/src/lib.rs Outdated Show resolved Hide resolved

Stranger6667 reviewed Jul 12, 2020

View reviewed changes

wasm/Cargo.toml Show resolved Hide resolved

refactor: merge imports from std

ed95670

Co-authored-by: Dmitry Dygalo <Stranger6667@users.noreply.github.com>

cheapsteak force-pushed the chang/20/07/attempt-at-wasm branch 2 times, most recently from 0b1cfc6 to 7badd7c Compare July 12, 2020 22:52

cheapsteak added 3 commits July 12, 2020 18:54

refactor: move gitignore into wasm crate

e3d7204

Signed-off-by: Chang Wang <garnwraly@gmail.com>

refactor: code cleanup

d81df50

Signed-off-by: Chang Wang <garnwraly@gmail.com>

build: add wasm release optimizations

f19f92f

Signed-off-by: Chang Wang <garnwraly@gmail.com>

cheapsteak force-pushed the chang/20/07/attempt-at-wasm branch from 7badd7c to f19f92f Compare July 12, 2020 22:54

feat: explicitply provide typescript typings

8ed2cfb

Signed-off-by: Chang Wang <garnwraly@gmail.com>

cheapsteak force-pushed the chang/20/07/attempt-at-wasm branch from c2adb54 to 8ed2cfb Compare July 13, 2020 00:18

Stranger6667 approved these changes Jul 13, 2020

View reviewed changes

Stranger6667 merged commit ff4fe58 into Stranger6667:master Jul 13, 2020

Stranger6667 mentioned this pull request Jul 13, 2020

WASM support #49

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initial attempt poc at wasm #47

initial attempt poc at wasm #47

cheapsteak commented Jul 11, 2020

Stranger6667 commented Jul 11, 2020

Stranger6667 commented Jul 11, 2020

Stranger6667 Jul 12, 2020

Stranger6667 Jul 12, 2020

Stranger6667 Jul 12, 2020

cheapsteak Jul 12, 2020 •

edited

cheapsteak Jul 12, 2020 •

edited

Stranger6667 Jul 12, 2020

Stranger6667 Jul 12, 2020 •

edited

cheapsteak Jul 12, 2020

Stranger6667 Jul 12, 2020

cheapsteak Jul 12, 2020

Stranger6667 Jul 12, 2020

cheapsteak Jul 12, 2020

Stranger6667 commented Jul 12, 2020

Stranger6667 commented Jul 12, 2020

Stranger6667 left a comment

Stranger6667 left a comment

cheapsteak commented Jul 13, 2020

initial attempt poc at wasm #47

initial attempt poc at wasm #47

Conversation

cheapsteak commented Jul 11, 2020

Stranger6667 commented Jul 11, 2020

Stranger6667 commented Jul 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cheapsteak Jul 12, 2020 • edited

Choose a reason for hiding this comment

cheapsteak Jul 12, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stranger6667 Jul 12, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stranger6667 commented Jul 12, 2020

Stranger6667 commented Jul 12, 2020

Stranger6667 left a comment

Choose a reason for hiding this comment

Stranger6667 left a comment

Choose a reason for hiding this comment

cheapsteak commented Jul 13, 2020

cheapsteak Jul 12, 2020 •

edited

cheapsteak Jul 12, 2020 •

edited

Stranger6667 Jul 12, 2020 •

edited