bytecodealliance / ComponentizeJS

JS -> WebAssembly Component
Apache License 2.0
225 stars 27 forks source link

add option to retain fetchEvent handler #116

Closed karthik2804 closed 2 months ago

karthik2804 commented 3 months ago

This PR adds the ability to use the fetchEvent Handler provided by StarlingMonkey.

fixes #114

karthik2804 commented 3 months ago

I think this should wait until we get the fetch PR, so leaving it as a draft.

guybedford commented 2 months ago

Please let me know if I can give any further feedback on this PR, or if you want to arrange a sync to discuss further @karthik2804.

karthik2804 commented 2 months ago

@guybedford I think this PR can be closed now as #117 enables the use of the fetch event handler as long as the guest content does not export the incomingHandler object.

karthik2804 commented 2 months ago

Checking back on this - this is not solved by #117 as we decided to remove changes affecting exports. Are we okay to still follow the plan described in https://github.com/bytecodealliance/ComponentizeJS/pull/116#discussion_r1658935176 but narrow it down to only the case of wasi-http export? or do we want to go down this route of adding it as a feature as is currently in this PR.

karthik2804 commented 2 months ago

The above is due to us exiting during the exports_bindgen call here and we do not even get to the point of optionally removing the export here

tschneidereit commented 2 months ago

I think we should still pursue the plan described in that comment, yes. Based on our conversation just now, that'll require parsing the input .wasm file earlier in the process so we can check whether it has the export in question. That seems okay to me, assuming it's not too challenging to do.

guybedford commented 2 months ago

I haven't tested the workflows here, but the code path in https://github.com/bytecodealliance/ComponentizeJS/blob/main/crates/spidermonkey-embedding-splicer/src/bindgen.rs#L383 no longer does any skipping at all.

So we are just always leaving the incoming handler export in the final binary, even though it is not reflected in the encoded component.

Therefore it should be possible to simply target a world with the incoming handler to get it handled correctly in the component encoding. It should be possible to test this at the component level by stripping the component encoding and reencoding it with the exported hanlder.

Assuming I've got the above right, then the issue is that we don't use the guest exports to inform whether to use the JS handler or the engine handler when it is present in the world, and instead we will always assume a JS handler thereby overriding the engine one. The way to filter this would be to have an exception for this specific function in that codepath and I'd be fine with that.

karthik2804 commented 2 months ago

@guybedford I believe you are correct in that the JS handler always overrides the Engine handler. To make sure I understand, are you suggesting that we make an exception just for the HTTP handler specifically with something like before this

if (name == "wasi:http/incoming-handler@0.2.0") {
  continue;
}

Alternatively, we could potentially parse the engine earlier and pass in the list of exports it is okay to ignore like @tschneidereit suggests.

I am happy with the first approach as well since that is simpler. Happy to open a new PR once an approach is decided. Thanks!

guybedford commented 2 months ago

Something like that sounds like it would work to me, yes.

karthik2804 commented 2 months ago

@guybedford do you have a preference between adding an exception for specific export or making it more general by parsing the engine before we get to the export_bindgen

guybedford commented 2 months ago

Since this is just for guest exports (since imports always coalesce so there are no conflicts to deal with) and incoming HTTP is the only one currently, hard-coding should be fine.

guybedford commented 2 months ago

Supported in https://github.com/bytecodealliance/ComponentizeJS/pull/122.