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

Add js/rust bindings for inline source map generation #62

Merged
merged 3 commits into from
Jan 30, 2024

Conversation

vstefanovic97
Copy link

Fixes #61

Right now I just added an extra argument to make it a non-braking change, but if you feel like we want to have second argument be an object that accepts the filename: string and input_source_maps: boolI can also change it to that

@ef4
Copy link
Collaborator

ef4 commented Jan 23, 2024

Thanks.

I think the suggestion of changing to an options object and accepting a breaking change is good, please update to that version.

@vstefanovic97
Copy link
Author

vstefanovic97 commented Jan 23, 2024

@ef4 this is my first time reading and writting rust so I gave it my best shot, as far as I could tell it doesn't support to have an optional object as an argument in rust, so I took care of that in js space and made sure to always provide all arguments with defaults to the function we get back from wasm.

As I don't have knowledge of rust code maybe there was a simpler solution, but this was the best I could do w/o spending weeks learning rust and wasm :)

Let me know if you have any feedback

Copy link
Contributor

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

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

Left some comments about the usage of node's wasm output in the standalone package 🙃

pkg/standalone.js Outdated Show resolved Hide resolved
@@ -35,6 +35,7 @@ lazy_static = "1.4.0"
base64 = "0.21.4"
serde = { version = "1.0", features = ["derive"] }
serde_json = "1.0"
serde-wasm-bindgen = "0.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

why this change?

Copy link
Author

Choose a reason for hiding this comment

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

I use it's from_value function to parse the object argument here

Copy link
Contributor

Choose a reason for hiding this comment

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

is it a successor to regular wasm-bindgen? or are the non-serde deps just on their way out anyway?

Copy link
Author

Choose a reason for hiding this comment

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

image
I got this from the docs, that's why I used it https://docs.rs/serde-wasm-bindgen/latest/serde_wasm_bindgen/.

Copy link
Author

Choose a reason for hiding this comment

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

tbh I didn't really have time to learn wasm or rust enough to properly understand what I was doing

Copy link
Contributor

Choose a reason for hiding this comment

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

oooo that's nice, thanks for providing that info!

Copy link
Contributor

@NullVoxPopuli NullVoxPopuli left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -173,7 +173,7 @@ describe(`parse`, function () {
p.process(
`const thing = "face";
<template>Hi`,
"path/to/my/component.gjs",
Copy link
Contributor

Choose a reason for hiding this comment

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

is the old way still able to be supported?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Upthread I said a breaking change is OK, so I don't think we should keep the old way. It's a very easy upgrade.

Copy link
Contributor

Choose a reason for hiding this comment

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

excellent!

Copy link
Member

@mansona mansona left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@vstefanovic97
Copy link
Author

@ef4 does it look good to you, can this get merged/released?

@ef4 ef4 merged commit 6e5c5a1 into embroider-build:main Jan 30, 2024
3 checks passed
@github-actions github-actions bot mentioned this pull request Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature request] source map generation
4 participants