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

Remove dependency renaming to enable WASM #621

Closed
wants to merge 3 commits into from

Conversation

alubbe
Copy link
Contributor

@alubbe alubbe commented Feb 3, 2020

@anurbol is this what you had in mind for the follow-up PR? #106 (comment)

ecmascript/src/lib.rs Outdated Show resolved Hide resolved
@alubbe
Copy link
Contributor Author

alubbe commented Feb 3, 2020

Regarding the failing fmt test - I'm using this command, why are the outputs different from what CI would like?

cargo +nightly fmt -- --unstable-features --skip-children

@kdy1
Copy link
Member

kdy1 commented Feb 3, 2020

Regarding the failing fmt test - I'm using this command, why are the outputs different from what CI would like?

cargo +nightly fmt -- --unstable-features --skip-children

You need to add --all to run cargo fmt on all crates.

@anurbol
Copy link
Contributor

anurbol commented Feb 3, 2020

Wow, great work. As kdy already mentioned, yep it seems that you've forgotten to add aliases to the imported names (just like in javascript - you can't just rename imported variable, you should assign the old name as the alias in order for things to keep working, otherwise, to rename all usages, which is more time-consuming)

@kdy1
Copy link
Member

kdy1 commented Feb 3, 2020

It seems like cargo fmt --all is required

@kdy1
Copy link
Member

kdy1 commented Feb 3, 2020

Btw, it's a breaking change as wasm module cannot be loaded synchronously, so transformSync and transformFileSync cannot be used.
How do you think about publishing it with a different name?

@alubbe
Copy link
Contributor Author

alubbe commented Feb 3, 2020

I think swc-wasm should be fine

@@ -1,6 +1,6 @@
use std::path::Path;
use swc::{
ecmascript::{
swc_ecmascript::{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be ecmascript

@@ -2,11 +2,11 @@ use swc;

use std::{path::Path, sync::Arc};
use swc::{
common::{
config::Options,
swc_common::{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be common

pub use sourcemap;
pub use swc_atoms;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be pub use swc_atoms as atoms

@anurbol
Copy link
Contributor

anurbol commented Feb 3, 2020

wasm module cannot be loaded synchronously

@kdy1 Hi, are you sure? new WebAssembly.Module() is synchronous.

@kdy1
Copy link
Member

kdy1 commented Feb 3, 2020

wasm module cannot be loaded synchronously

@kdy1 Hi, are you sure? new WebAssembly.Module() is synchronous.

Oh.. I didn't know that. Then if wasm's performance is not bad, I'll change @swc/core to use wasm I'll consider changing it to wasm.

(Edited as it does not seem like a simple decision)

I've searched about it, and it seems like the size of the synchronous wasm module is limited to 4kb. I'll publish it as @swc/wasm

@anurbol
Copy link
Contributor

anurbol commented Feb 3, 2020

I don't know if you are currently making and distributing binaries, but for me that's hell (all the ABI-compatibility quirks). Wasm is so much easier, it's breeze. That said, wasm is 2-3 times slower than native (at least for me, on Windows), but still incredibly faster than JS (in some tasks, e.g. processing strings, x30 faster).

@anurbol
Copy link
Contributor

anurbol commented Feb 3, 2020

I've searched about it, and it seems like the size of the synchronous wasm module is limited to 4kb.

Could you share the link to that info? Seems weird. I am loading a wasm module that's a few Mb (if we talk about .wasm file size) and it works fine.

If you do this, how are you going to sync changes and files between the two repos? Is there an easy way I am not aware of?

@kdy1
Copy link
Member

kdy1 commented Feb 3, 2020

@anurbol https://developers.google.com/web/updates/2018/04/loading-wasm
Hmm... Does the limitation exist only for browser?

@anurbol
Copy link
Contributor

anurbol commented Feb 3, 2020

@kdy1 I guess the limitation is only for browsers. For node.js (where SWC is supposed to be used, i guess) that limitation just doesn't make sense.

@anurbol
Copy link
Contributor

anurbol commented Feb 3, 2020

@kdy1 Also, I didn't even touch the WebAssembly API, I am just using Wasm-Bindgen and it generates the bindings, and they are 100% synchronous. And their book didn't mention anything about size limit.

@kdy1
Copy link
Member

kdy1 commented Feb 3, 2020

@anurbol It sounds great.

@alubbe
Copy link
Contributor Author

alubbe commented Feb 4, 2020

It was easier for me to work all these changes into a new PR - so I'm closing this in favour of #625

@alubbe alubbe closed this Feb 4, 2020
bors bot pushed a commit that referenced this pull request Feb 5, 2020
This PR supercedes #621 and tries to remove dependency renaming to enable WASM, as discussed in #106
@swc-project swc-project locked as resolved and limited conversation to collaborators Nov 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants