-
Notifications
You must be signed in to change notification settings - Fork 27
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 option to retain fetchEvent handler #116
base: main
Are you sure you want to change the base?
Conversation
ffc32b8
to
2d319cf
Compare
Signed-off-by: karthik2804 <karthik.ganeshram@fermyon.com>
2d319cf
to
ab1cd04
Compare
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.
Can we also add this to the readme?
src/componentize.js
Outdated
@@ -45,6 +45,7 @@ export async function componentize(jsSource, witWorld, opts) { | |||
worldName, | |||
disableFeatures = [], | |||
enableFeatures = [], | |||
retainFetchEvent = false |
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 wonder if it would make sense to integrate this into the enableFeatures
model and call it fetch-event
?
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.
Oooh! I like the idea of adding it via the enableFeatures
. I will also add it to the readme.
I think this should wait until we get the fetch PR, so leaving it as a draft. |
Signed-off-by: karthik2804 <karthik.ganeshram@fermyon.com>
@@ -130,6 +130,10 @@ Setting `disableFeatures: ['random', 'stdio', 'clocks']` will disable all featur | |||
|
|||
Note that pure components **will not report errors and will instead trap**, so that this should only be enabled after very careful testing. | |||
|
|||
The features that are not included by default are: | |||
* `'http'` - Support for sending and receiving HTTP requests, depends on `wasi:io` | |||
* `'fetch-event'` - Enables using `fetchEvent` to respond to `wasi:http/incoming-handler@0.2.0#handle`. If the target world does note export `wasi:http/incoming-handler@0.2.0`, this will be ignored. |
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.
Just like we automatically merge WASI imports into the target world, perhaps we can automatically merge the wasi:http/incoming-handler@0.2.0
export into the target world when this feature is used?
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.
Actually isn't this the default behaviour anyway? Or do we still strip the export if it's not explicitly in the target world?
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.
With this PR we want to only optionally strip the export from the StarlingMonkey engine here based on the value of the feature. My intention with that statement is to make it such that, if the target world does not export the interface but the feature is enabled, we should probably still strip the export.
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.
Shouldn't it read "If the target world does export" though? Rather than "does not". Thinking about this further we probably want to make this an explicit error. That is, if you enable this feature and the target world already exports the incoming handler, then we should throw an error that you should either target the incoming handler export, or you should use the fetch event version of it, but not both.
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 there is a disconnect in what we each mean by the target world. I am talking about the wit
definition, not the guest content. As in, consider the following world
package local:hello;
world hello {
export hello: func(name: string) -> string;
}
and the fetch-event
is set, if we do not strip the export from the engine, it will end up in the output component but maybe that is fine?
I also think that there is currently no way to check if a handler is attached to the fetch event but we can check if there is something that targets the incoming handler export so erroring on that would be good, I agree.
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.
In case of writing an app that will be used as a http trigger using the fetch event, wouldn't the host expect to call the export even though it is not part of the target world explicitly?
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.
If the developer is specifying a target world that is different from what their use case actually needs, and their runtime provides, then that's a bug in the configuration. With what Guy is proposing here, there'd be a higher likelihood of things sort of accidentally working out regardless. That could be seen as a downside, but I'm not even entirely sure it is, honestly.
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 am misunderstanding here. To verify my understanding - the proposal above is to not retain fetchEvent
incase the target world contains the wasi:http/incoming-handler
export and only have the feature take effect if it is not explicitly part of the target world.
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.
Oh, I see—I had missed that part of it. On that, I agree with you: I think this setting should be usable whether the targeted world includes incoming-handler
or not.
@guybedford doing what you proposed would make it substantially harder to use FetchEvent
, because now anyone who wants to target an environment that supports wasi:http/proxy
has to edit the target world not to include incoming-handler
if they want to use FetchEvent
. I think that's a requirement we really shouldn't put on people—and it'd be quite hard for us to satisfy using Spin as a development tool.
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 am happy to update this PR once there is consensus on this discussion.
This PR adds the ability to use the
fetchEvent
Handler provided byStarlingMonkey
.fixes #114