Skip to content
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

Help wanted: create a Wasm example/tutorial using rust_xlsxwriter #38

Closed
jmcnamara opened this issue May 7, 2023 · 25 comments
Closed

Comments

@jmcnamara
Copy link
Owner

jmcnamara commented May 7, 2023

Help wanted

It would be great if someone could create a Wasm example/tutorial using rust_xlsxwriter.

Ideally I could add this as a section in or link from the Working with the rust_xlsxwriter library user guide.

An example like one in the SheetJS tutorial would be good but don't copy it exactly.

It would be good to use the write_row_matrix() or write_column_matrix() methods that are currently on main (#16) and which will be in v0.39.0.

It would also be nice to use whatever is the current Rust/Wasm best practices.

@jmcnamara jmcnamara added question This is a general question help wanted and removed question This is a general question labels May 7, 2023
@jmcnamara jmcnamara changed the title Help wanted: create a WASM example/tutorial using rust_xlsxwriter Help wanted: create a Wasm example/tutorial using rust_xlsxwriter May 7, 2023
@jmcnamara
Copy link
Owner Author

jmcnamara commented May 12, 2023 via email

@robothot
Copy link

@jmcnamara Any plans? For example

  1. Create a project, or create a folder on this project ?
  2. Documentation site is using docusaurus or dumi

I think we or maybe need some planning ?

@jmcnamara
Copy link
Owner Author

Any plans?

I think the best approach would be to create a separate GitHub project and get some examples working with Javascript.

Then, once it it working, and we get some user testing/feedback, I will add a chapter in the rust_xlsxwriter User Guide with links/code from the Wasm project.

@robothot
Copy link

ok

@Ashci42
Copy link

Ashci42 commented May 26, 2023

Hey, was working on a rust lib with python and wasm bindings and noticed this issue. In my case, I couldn't get rust_xlswriter to work with wasm because of time not being implement on that platform (yet). I believe this is due to your usage of the chrono crate. It looks like they have a wasmbind feature flag that should make it work in WebAssembly (for reference).

Could the feature flag be added for chrono?

@jmcnamara
Copy link
Owner Author

Could the feature flag be added for chrono?

It could. I could hide all the date/time types behind IntoDateExcelTime/IntoExcelDate/IntoExcelTime traits and implement it for the chrono types (under a feature flag) and some other tuple/string types with a fallback to native/internal date/time handling. I already have functions like that in the C version of the library.

This would give the user some access to date/time handling (which is important in Excel) but also chrono if they wanted it. This would also remove, or at least make an option of, a dependency which would be good.

Note, this part of the request is similar to #6.

@jmcnamara
Copy link
Owner Author

Could the feature flag be added for chrono?

@Ashci42 I have made chrono an "optional on" feature in rust_xlsxwriter v0.41.0 and it will be "optional off" in future version (and current main). I've added the ExcelDateTime struct as a native alternative.

@Clipi-12
Copy link

Clipi-12 commented Aug 23, 2023

I don't think it is currently possible to run rust_xlsxwriter with Wasm.

Calling Workbook::new() calls DocProperties::new(), which itself calls ExcelDateTime::utc_now(). This method calls std::time::SystemTime::now(), which panics on runtime in Wasm.
Perhaps we could make DocsProperties.creation_time an Option that would be lazily evaluated to the default value if it isn't set by the user. For this to work, Workbook would also need to be able to be created with a user-defined DocProperties, instead of just calling DocProperties::new().

I also think that packager::assemble_file will probably panic, as it calls std::thread::(scope::)spawn, which I don't think is implemented in Wasm.

@jmcnamara
Copy link
Owner Author

I don't think it is currently possible to run rust_xlsxwriter with Wasm.

Thanks for taking a look at this.

Calling Workbook::new() calls DocProperties::new(), which itself calls ExcelDateTime::utc_now(). This method calls std::time::SystemTime::now(), which panics on runtime in Wasm.

Excel has metadata with a creation and modification data (which are the same for a new file). It is possible to specify a datetime so that ExcelDateTime::utc_now() isn't called like this:

use rust_xlsxwriter::{DocProperties, ExcelDateTime, Workbook, XlsxError};

fn main() -> Result<(), XlsxError> {
    let mut workbook = Workbook::new();

    // Create a file creation date for the file.
    let date = ExcelDateTime::from_ymd(2023, 1, 1)?;

    // Add it to the document metadata.
    let properties = DocProperties::new().set_creation_datetime(&date);
    workbook.set_properties(&properties);

    let worksheet = workbook.add_worksheet();
    worksheet.write_string(0, 0, "Hello")?;

    workbook.save("properties.xlsx")?;

    Ok(())
}

See: https://rustxlsxwriter.github.io/workbook/checksum.html#checksum-of-a-saved-file

However, I don't know if it is still incompatible with Wasm if the ExcelDateTime::utc_now() function is still in the compile path even if not in the execution path.

I also think that packager::assemble_file will probably panic, as it calls std::thread::(scope::)spawn, which I don't think is implemented in Wasm.

That was only added in recently in v0.44.0. You could test with version v0.43.0 instead. If required I could put it behind a wasm feature flag.

@jmcnamara
Copy link
Owner Author

jmcnamara commented Aug 23, 2023

It is possible to specify a datetime so that ExcelDateTime::utc_now() isn't called like this:

Actually I'm wrong. It is called at the creation of DocProperties. However, I could make that an Option<> or also hide it behind a feature flag. It might matter to some folks if the creation time isn't actually the creation time.

@Clipi-12
Copy link

Clipi-12 commented Aug 23, 2023

I have forked the repo (here) changing those two things (making DocsProperties.creation_time an Option and putting std::thread::spawn under a cfg attribute) and everything seems to work so far. I'm afraid that I don't have a simple example to show that it actually works, though.

It might matter to some folks if the creation time isn't actually the creation time.

Yeah, that makes sense. Although maybe a DocProperties::new_lazy method could be added so that DocProperties::new does what it does now (plus wrapping DocsProperties.creation_time in Some), and therefore we have both methods without an additional feature flag

@jmcnamara
Copy link
Owner Author

I have forked the repo (here) changing those two things (making DocsProperties.creation_time an Option and putting std::thread::spawn under a cfg attribute) and everything seems to work so far.

Thanks for that.

For the ExcelDateTime::utc_now() issue I think a more complete solution would be to fall back to a JS function on wasm targets like Chrono does. I could implement a wasmbind feature flag like Chrono and pass it to Chrono when that is in use. (I now see that this is what @Ashci42 was asking for above but I missed the point.) I'll also use it to make ExcelDateTime JS compliant and to turn off the back end threading.

I'm afraid that I don't have a simple example to show that it actually works, though.

That is still the main help I need here. It doesn't have to be full tutorial, like I asked for above. Even a hello world example with some instructions would be useful to debug and test issues like this.

@jmcnamara
Copy link
Owner Author

jmcnamara commented Aug 24, 2023

@Clipi-12 (or @Ashci42) I have pushed a (hopefully) Wasm compatible version to the wasm branch if you can test it.

I don't know if it also needs some target_arch configs. You can let me know.

I need to add support for the wasm flag into the chrono flag as well. I'll add that a bit later.

@Clipi-12
Copy link

Clipi-12 commented Aug 24, 2023

I'll take a look at this.

In the meanwhile, it could be important to think about Wasm in non-javascript environments. I'm not an expert on this, but I know Wasm was "designed to be capable of being executed without a Javascript VM present".

Also, if we look at chrono's Utc::now() implementation they don't call js_sys unless they absolutely need to (i.e. when wasm32-unknown-unknown is used). In the case that wasi or emscriptem are used, chrono doesn't use js_sys or anything javascript-related, (presumably) so that the Wasm code doesn't need a javascript environment.

@jmcnamara
Copy link
Owner Author

jmcnamara commented Aug 24, 2023

Also, if we look at chrono's Utc::now() implementation they don't call js_sys unless they absolutely need to

Yes, you are probably right. I had a look at the time.rs crate and they do the same thing. I'll update the branch. -- Done.

@Clipi-12
Copy link

I have made a minimal example of a rust_xlsxwriter program that compiles to WASM:
https://github.com/Clipi-12/rust_xlsx_wasm_example
It can be used to compile and test in a browser environment, in Nodejs, in Deno, and in WASI.

It is a bit big, but there's not much I can do, since most files are just there to replace std::fs with browser-specific/nodejs-specific/deno-specific code that does the same thing.

I also found a bug in the current branch. You are taking the result of Date.now() as if it meant seconds since UNIX_EPOCH, while it actually represents the number of milliseconds since UNIX_EPOCH (so you just have to divide it by 1,000 and it would be fixed)

@jmcnamara
Copy link
Owner Author

That is really great. Thanks!

I was able to get the browser example working with minimal fuss. That and the other environments make it perfect for testing so that is just what I needed.

I also found a bug in the current branch

I fixed that on the wasm branch and force pushed it.

@jmcnamara
Copy link
Owner Author

It can be used to compile and test in a browser environment, in Nodejs, in Deno, and in WASI.

I was able to get all of the targets (Browser, Node.js, Deno and WASI) working after installing a few dependencies. So that is really useful for testing.

It is a bit big, but there's not much I can do, since most files are just there to replace std::fs with browser-specific/nodejs-specific/deno-specific code that does the same thing.

It doesn't seem too bad.

I have a few minor suggestions. I'll make them directly on your repo a bit later.

Thanks once more.

@jmcnamara
Copy link
Owner Author

@Clipi-12 I added support in rust_xlsxwriter for wrapping wasm_bindgen::JsValue into XlsxError. That should simplify the code and make handling errors in the wasm app(s) easier since it already wraps IO::Error.

I've pushed it to a different branch (wasm2) since it doesn't work for wasmtime which gives a generic and not very clear error:

$ wasmtime --dir=. pkg/wasi/rust_xlsx_wasm_example.wasm
Error: failed to run main module `pkg/wasi/rust_xlsx_wasm_example.wasm`

Caused by:
    0: failed to instantiate "pkg/wasi/rust_xlsx_wasm_example.wasm"
    1: unknown import: `__wbindgen_placeholder__::__wbindgen_throw` has not been defined

@Clipi-12
Copy link

Clipi-12 commented Aug 28, 2023

EDIT: never mind, I understand what you meant with the last commit. It is indeed very useful! I'll probably remove some code in the example thanks to that

@Clipi-12
Copy link

Clipi-12 commented Aug 28, 2023

@jmcnamara I modified a private copy of the wasm2 branch and got rid of the error by simply avoiding to import wasm-bindgen in wasi. Honestly I think the best option is to rename the wasm feature to javascript, since wasi already does all the thing we are trying to do with javascript, but it does them natively.

Well, everything but std::thread:spawn, which doesn't really need the feature-flag check anyways, since it can use #[cfg(target_arch = "wasm32")]

Also, an additional benefit is that if we rename the feature flag to javascript we don't have to do the long check #[cfg(all(target_arch = "wasm32", not(any(target_os = "emscripten", target_os = "wasi"))))] since we are guaranteed that js_sys is going to work

Clipi-12 added a commit to Clipi-12/rust_xlsx_wasm_example that referenced this issue Aug 28, 2023
@jmcnamara
Copy link
Owner Author

jmcnamara commented Aug 28, 2023

never mind, I understand what you meant with the last commit. It is indeed very useful! I'll probably remove some code in the example thanks to that

That is good. I meant to attach the updated example but somehow failed to do it, but it looks like you figured it out anyway. :-) I'll have a look at the changes a bit later.

Apologies for slow responses, I have some other things going on at the moment.

@jmcnamara
Copy link
Owner Author

jmcnamara commented Sep 1, 2023

I got a chance to look at this again and your recent changes look good. I've cleaned things up on my side and merged the changes onto main. I'll push the changes to crates.io in the next few days if everything is okay on your side and mine.

Honestly I think the best option is to rename the wasm feature to javascript,

I'm going to stick with wasm for now since I think that makes most sense in terms of what what users will be looking for.

Well, everything but std::thread:spawn, which doesn't really need the feature-flag check anyways, since it can use #[cfg(target_arch = "wasm32")]

Good point. I made that change.

Also, an additional benefit is that if we rename the feature flag to javascript we don't have to do the long check

I don't really get this. Could you explain a bit more. Or submit a PR against main with the suggested changes.

@Clipi-12
Copy link

Clipi-12 commented Sep 1, 2023

I don't really get this. Could you explain a bit more. Or submit a PR against main with the suggested changes.

I was about to submit a PR when I noticed that it actually doesn't matter. I thought that we could ignore the target_arch and target_os as long as the wasm feature flag was set because of the panicking we faced when running in non-javascript targets. I thought that we could assume we were in a js environment because otherwise it would still panic elsewhere.

However, the panic no longer occurs, and even if it did, it's honestly more safe to just do the check, in case it would ever get solved (which it already is).

@jmcnamara
Copy link
Owner Author

Thank for that @Clipi-12

I've pushed the changes to crates.io in v0.47.0.

I've mentioned your repo in the release notes: https://rustxlsxwriter.github.io/changelog.html#0470---2023-09-02

Thanks once more for the help. There will probably be some improvements to be added over time (in both repos) but this is a good start.

I'm going to close this feature request since I think it completes the "Help needed" request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants