Start using webext-messenger (for background functions and messages)

[fregante]

Related:

• Implement core background-content script communication + automated tests webext-messenger#2
• Never keep method the code next to the the caller webext-messenger#9

NB: By necessity and by design, this pattern is more verbose than the lift pattern since we intentionally keep the implementation separate from the caller. More context in:

• Prevent context-specific code from being imported in the wrong context #669

This PR:

• I redesigned the messenger’s API several times looking for the pattern that offered the most constraints and was less error prone, but some rules just can't be enforced (unless I write a lint rule, maybe)
• I ported a few background functions to this new pattern, including some that were plain sendMessage calls
• API and registration always happens in 2 files (per context)
• Implementations (the code that is actually executed in the background) are currently either in their previous location or in "executor". They don't need to be there, I just kept them there for diff clarity.
• No functionality was changed (with 2 exceptions I mention in my review), so large diffs are just due to indentation changes
• I tested most of the methods, except uninstallContextMenu and activateTab, which I'm not sure how to trigger exactly since tab is already active

Future work:

• Implement background page -> content script communication webext-messenger#10
• As I add more targets, I don’t think the API will change much, especially for methods that don't make use of the meta. Either way the changes will be easy once they follow one pattern.

[fregante]

I moved the contents of this function to the only place where it was called, to avoid too much redirection. This readiness part can likely be further simplified over time

[fregante]

This applies to every single message and it's now being automatically checked by webext-messenger itself:

https://github.com/pixiebrix/webext-messenger/blob/c98e5d1714032662574970ad485e5922cae9e561/index.ts#L49

[twschiller]

If you define a literal of what you passed to registerMethods, shouldn't you be able to have typescript generate the type from the literal instead of having to retype it all?

Updated: I wrote a comment in slack after I looked over the library code

[fregante]

That's exactly what we're trying to avoid in #669

If you specify the object:

const handlers = {
  UNARY: unary
}

registerMethods(handlers);

With the intent of using the type in the caller context, webpack will definitely include the unary function in both contexts.

[twschiller]

I'm really not a fan of "overloading" this to mean the messenger. this has a special meaning in OO languages

[twschiller]

Thoughts on toErrorResponse, isErrorResponse, etc? Are they unnecessary serialization?

Re-reading the code, it looks like they're self contained within the handlers, so if we don't need the serialization they should be safe to drop if we don't think serialization is necessary

[fregante]

The sendMessage API indeed already serializes errors but:

• Wrapped errors from sendMessage should include the original error name, stacktrace mozilla/webextension-polyfill#210
• said serialization might still be needed once we add the port messaging

So I think in order to maintain stacktraces of errors that happened in the background I'll have to implement the error serialization now

[twschiller]

I'm not sure there's much benefit in having different code paths for the browser here

[fregante]

Sure, I just preserved the existing logic:

pixiebrix-extension/src/utils/permissions.ts

Lines 54 to 58 in 5d181f7

	if (browser.permissions) {
	return browser.permissions.contains(permissions);
	}
	return containsPermissionsInBackground(permissions);

However I think it's still worth doing because:

• it's a simple API wrap
• Chrome users (the majority) win in perf
• we still need a comment to explain why we're using a background call
• it reminds us we will probably be able to drop this background call at some point