-
Notifications
You must be signed in to change notification settings - Fork 2
Create WASM bindings and wire up into the vanilla JS Mapbox demo #106
Conversation
Ah right, my reference was https://rustwasm.github.io/wasm-bindgen/reference/arbitrary-data-with-serde.html |
@@ -21,3 +21,8 @@ pub mod transform; | |||
pub use transform::{lanes_to_tags, tags_to_lanes, LanesToTagsConfig, TagsToLanesConfig}; | |||
|
|||
mod test; | |||
|
|||
#[cfg(target_arch = "wasm32")] |
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 think another crate is overkill; it's not too much work to feature-gate a few dependencies and this modul
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.
maybe, I still think the separation helps in readability and separation of concerns?
I will do that work as part of #60, if I still think it is needed.
It also has the added benefit of being a home for node js based integration tests
<script type="module"> | ||
import init, { js_tags_to_lanes } from "./pkg/osm2lanes.js"; | ||
|
||
await init(); |
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 not convinced this is correct; I think everything here needs to be in an async function
. But it kind of works out as long as the user doesn't click a road too quickly before this happens to finish
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 think top level await is a thing (though I personally haven't used module scripts directly in the browser before). So this should work, but at the cost of queuing the rest of the setup work behind init()
.
You could kick off init()
here, store the promise, and await
it just before you use the lib (on line 120).
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.
Hmm, then we need extra state to avoid awaiting the promise multiple times. Cool that top-level await works directly though! I'll keep this in mind if we hit issues later
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.
FYI, awaiting the promise multiple times is also safe, at the cost of yielding to the event loop (like setTimeout( _, 0)
).
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.
Looks OK, just need to figure out how to integrate this into the deployed website.
@@ -21,3 +21,8 @@ pub mod transform; | |||
pub use transform::{lanes_to_tags, tags_to_lanes, LanesToTagsConfig, TagsToLanesConfig}; | |||
|
|||
mod test; | |||
|
|||
#[cfg(target_arch = "wasm32")] |
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.
maybe, I still think the separation helps in readability and separation of concerns?
I will do that work as part of #60, if I still think it is needed.
It also has the added benefit of being a home for node js based integration tests
Rebasing...
Sounds good to me. I'm still not sure what the node config looks like, or how to wire up end-to-end JS integration tests. This is just a small step in that direction |
This breaks
|
Argh, I failed to test everything. Feel free to revert temporarily to unblock working on that, if you need to. Work and travel stuff mean I probably can't touch anything in osm2lanes until after April 6 or so. :\ |
Fixed in #110 I still don't know why it failed, probably trunk fiddling with wasm-bindgen and ony expecting it to exist in the root of the tree. |
I believe this is the minimal work needed to call the Rust library from JS. It appears to work:
Still TODO: passing more options in, error handling, and an NPM package