More granular features for stubbing

[vados-cosmonic]

During a recent PR that patched a bug in feature-based WASI interface stubbing, the point of how to deal with outgoing-handler came up:

#268 (comment)

It seems like there are at least two solutions that could improve the ergonomics of stubbing:

• stubbing out fetch() itself via generated prelude to delete fetch
• more granular features to enable stubbing

Implementing this and avoiding introducing bugs probably requires a bunch of upfront work:

• Adding tests for all the current features & their usage (making sure existing stub combinations work)
• implementing the prelude changes
• adding & implementing the new granular features

Some of this work may overlap (and possibly resolve) #250

[tschneidereit]

I think we can get away without more granular, and thus harder to use, feature flags. What I was talking about regarding the global fetch function is not to stub it out, but to actually delete it from the global object. That way, it's simply not available to call at all, and thus doesn't mislead developers.

[vados-cosmonic]

about regarding the global fetch function is not to stub it out, but to actually delete it from the global object. That way, it's simply not available to call at all, and thus doesn't mislead developers.

Ah, this I understood -- I responded in the original comment chain with this:

The problem with (1) is that even if we stub out the fetch global function, the component being loaded might still try to use it right? Which turns into a runtime error regardless (which IMO is what you were trying to avoid?).

Removing the global will still cause runtime errors, correct? What's was the benefit we were going for there? If we're replacing it with a more descriptive error then I could understand (essentially fetch = () => new Error('...');) but I'm not sure I get what the improvement is from simply removing

[tschneidereit]

I'm not sure I get what the improvement is from simply removing

Right now, calling fetch with things being stubbed out will cause a Rust panic and abort execution. That's something content should ideally not be able to trigger at all. If instead we delete fetch, then trying to call it causes a JS exception, which can be caught, analyzed with normal tooling, etc.

[vados-cosmonic]

Yeah good point -- the user has to have been lucky enough to essentially put fetch inside a try though -- usages like fetch().then().err() would fail and crash and abort execution.

I think we might benefit more from replacing fetch with an immediately Promise rather than just removing it.

[tschneidereit]

usages like fetch().then().err() would fail and crash and abort execution

The crucial difference is that these patterns do not actually crash. As in, they are properly handled by StarlingMonkey, if not content. That's a pretty key difference in my mind—in big part because it prints a JS stack trace, instead of a very unhelpful panic message and the (usually unsymbolized) wasm stack.

I do think not having fetch available is just the right thing to do here. Same as for any other functionality not added to the configuration.

[vados-cosmonic]

Hmnn sure I could see that -- I think I'm more focused on regardless of the reason for the failure (which we're in control of), being clear about the reason of the failure to the user. That said, if a user ran into this issue we can hope that they would wonder why fetch wasn't defined and go look back at their configuration.

I do think not having fetch available is just the right thing to do here. Same as for any other functionality not added to the configuration.

Sure, I can certainly live with this!

[tschneidereit]

A key reason why I think it just shouldn't be there is that that enables feature testing along the lines of if (!globalThis.fetch) { ... }

[vados-cosmonic]

Oh that's a good point -- adding it would definitely make it hard to detect whether it was enabled/disabled, and encouraging checking for globalThis.fetch before use is a reasonable general suggestion, though I think most don't do that by default (but maybe they should).

[tschneidereit]

Oh, I fully agree that most won't. And I'm not even sure that most should, honestly: in most situations if you write code using fetch you can be confident that it'll run in an environment that actually has it. I guess my main consideration in all of this is that fetch isn't special: there are lots of things a developer could conceivably think will be available, and in many cases will think are available, such as Node.js builtins. Given that we're not doing anything special for any of those, I don't think we should for fetch purely because we're now introduction a situation in which it'll not be available.