-
Notifications
You must be signed in to change notification settings - Fork 93
feat: publish xlsx import export as separate wasm #381
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
base: main
Are you sure you want to change the base?
feat: publish xlsx import export as separate wasm #381
Conversation
3fe119d
to
b9f70b0
Compare
Hi @BrianHung, This is definitely a good idea! And people have asked for this. Lets do it. But I am not sure if we can use the xlsx crate as is, as that crate might grow in considerable size. That being said my first problem is that building the wasm as per your instructions I actually get a smaller build! What am I doing wrong?
|
zip = "0.6" | ||
zip = { version = "0.6.6", default-features = false, features = ["deflate", "time"] } | ||
flate2 = { version = "1.0", default-features = false, features = ["rust_backend"] } | ||
time = { version = "0.3", features = ["wasm-bindgen"] } |
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.
Could you explain why we need all this?
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.
xlsx are actually zip files, so we need to bundle the utility functions flate2
(for deflating a zip file) and time
(timestamps in headers) along with it.
Or is it that you need both wasm files? That is an awesome feat! I was imagining you would have two builds:
I have done that in the past and discarded for now as I didn't see an immediate benefit for IronCalc. But having two different packages is interesting as you could load the second one on demand only when importing/exporting. |
|
||
fn main() { | ||
// This is required for cargo to compile this as a binary | ||
} |
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'm a bit curious about this, why didn't you go the same way as the other build? Note in Cargo you can have different build targets. See for instance in IronCalc:
https://github.com/ironcalc/IronCalc/blob/main/base/Cargo.toml#L24-L27
https://github.com/ironcalc/IronCalc/blob/main/base/src/model.rs#L38-L64
Maybe that would make things easier? We just have two builds with different features?
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 is so we can have one crate, but two wasm files outputted.
If there isn't a main declaration, cargo would just treat it as a library and won't turn it into an artifact.
I'm pretty new to rust, so if you have a preferred method that achieves the same goal, I would defer to you here!
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.
Hi @BrianHung, I left a few questions, mostly for my own understanding. Once I understand what is going on I would like to streamline the process a bit. But I think this is awesome!
Yep, that's right! We keep ironcalc and xlsx import/export as separate wasm files so we don't cause client to have an unnecessary larger bundle size if they don't use xlsx utilities. |
Separates xlsx into its own wasm file to address bundle size increase concerns in #379.
Had to update wasm-bindgen to avoid a separate crate just for xlsx.wasm.