Add background page receiver

[fregante]

• Fixes Implement core background-content script communication + automated tests #2

Related:

• Messaging architecture #1
• #578: [WIP/POC] messaging infrastructure pixiebrix-extension#1065
• First pass, test framework attempts #7

While automated tests might be out of reach for now, nothing stops us from at least have some semi-automated ones where we just have to open each context and look at the results in the console.

I'd like to start using this in the extension as soon as possible to see if it works for us. The only problem in that regard is a type issue for functions with parameters, can you look into it?

test/demo-extension/background-handlers.ts:11:52 - error TS2344: Type '(_: MessageSender, ...addends: number[]) => Promise<number>' does not satisfy the constraint 'Method'.

11 export const sumContract: Partial<Contract<string, typeof sum>> = {
                                                      ~~~~~~~~~~

test/demo-extension/background.ts:10:19 - error TS2345: Argument of type '(_: MessageSender, ...addends: number[]) => Promise<number>' is not assignable to parameter of type 'Method'.

10 addHandler('sum', sum);
                     ~~~

test/demo-extension/contentscript.ts:17:31 - error TS2345: Argument of type 'Partial<Contract<string, (_: MessageSender, ...addends: number[]) => Promise<number>, (...rest: number[]) => Promise<number>>>' is not assignable to parameter of type 'Partial<Contract<string, Method, (...rest: unknown[]) => Promise<unknown>>>'.
  Types of property 'method' are incompatible.
    Type '((_: MessageSender, ...addends: number[]) => Promise<number>) | undefined' is not assignable to type 'Method | undefined'.
      Type '(_: MessageSender, ...addends: number[]) => Promise<number>' is not assignable to type 'Method'.

17   const sum = createMessenger(sumContract);
                                 ~~~~~~~~~~~

[fregante]

To clarify the types a bit:

There's a Contract object (a real object) that is at least:

{
	type: "functionName"
}

This object might have:

• publicMethod, which is a type-only interface for the caller, used in createMessenger
• method, which is only used to typecheck the addHandler call and ensure that the assigned function matches the contract.

[fregante]

To run the tests, build the extension with:

npm run demo:watch

Then open it in the browser with the usual

web-ext run

and then open the console. You might need to refresh the page once.

---

To run TypeScript on the library:

npm run watch

For linter and Prettier at once:

npm run fix

[twschiller]

If you write as a constant you get a bit better error message

It's saying that the type is wrong because if: 1) I have something of type message, 2) I can't substitute sumMethod for it because the args might not be numbers

The types need to look more like this (but this is still not exactly right)

type ParametersExceptFirst<F> = F extends (
  arg0: any,
  ...rest: infer R
) => infer ReturnValue
  ? (...rest: R) => ReturnValue
  : never;
type BaseActionType = string;
type BasePayload<TParams extends unknown[] = unknown[]> = [browser.runtime.MessageSender, ...TParams];
export type Method<TParams extends unknown[] = unknown[]> = (...parameters: BasePayload<TParams>) => Promise<unknown>;
export type Contract<
  T extends BaseActionType = BaseActionType,
  TParams extends unknown[] = unknown[],
  M extends Method<TParams> = Method<TParams>,
  P extends ParametersExceptFirst<M> = ParametersExceptFirst<M>,
> = {
  type: T;
  method: M;
  publicMethod: P;
};

[twschiller]

publicMethod, which is a type-only interface for the caller, used in createMessenger
method, which is only used to typecheck the addHandler call and ensure that the assigned function > matches the contract.

I don't understand these fields, and there's no examples in the tests

[twschiller]

Why not call this payload? I think there's two ways to go:

• RPC-style naming, of operationId and args
• Message-style naming (a la https://github.com/redux-utilities/flux-standard-action): type and payload

Also, note the difference between parameters and arguments: https://developer.mozilla.org/en-US/docs/Glossary/Parameter

[fregante]

My idea is to only allow RPC-style messaging instead of exposing two interfaces (one for RPC and one for general messages, which isn't necessary if the first one works well).

Names aren't thought out yet, no strong preference since they're going to be on a private object, but since it's RPC-style-only it makes sense to make it an array.

[fregante]

I renamed everything to type and args to keep them short. type is the only one visible by the user (I don’t think we'll match the RPC method fully).

I think at some point we'll want to add a library-specific key so we can safely catch non-handled messages with it:

browser.runtime.onMessage.addListener(function onMessage(message) {
  if (!message.__messenger__) {
     // Completely ignore message, this was not sent by the library
     // and should not be handled by it
    return;
  }

  const handler = handlers.get(message.type);
  if (!handler) {
    // Even if we don't find the handler, we can safely throw an error,
    // because we're sure it was *supposed* to be handled
    throw new Error('No handler registered for ' + message.type);
  }

  return handler.call(sender, ...message.args);
});

[twschiller]

My idea is to only allow RPC-style messaging instead of exposing two interfaces

Sure happy to give it a try. In my experience array typing in TypeScript is more annoying that object typing. Arrays also historically are tricky for subtyping: https://en.wikipedia.org/wiki/Covariance_and_contravariance_(computer_science)#Arrays. I'm not familiar with how gracefully TypeScript handles subtyping arrays

The final thing I'll say in support of object payloads is it matches more closely to the browser's sendMessage API

From a functionality standpoint, they're equivalent. E.g., (arg1: Record), {args: unknown[]}. So I'd err on whatever gives the best TypeScript ergonomics.

The benefit of the args based approach I guess is that you can point it to any existing method... Is that what you're going for?

I think at some point we'll want to add a library-specific key so we can safely catch non-handled messages with it

I agree seems helpful

[fregante]

The final thing I'll say in support of object payloads is it matches more closely to the browser's sendMessage API

I'm confused by this. Arrays are objects and the sendMessage API accepts any serializable value, not just objects. Even primitives are accepted. The actual message is still a {t:string,a:array} object.

The only concern we have is about typing, i.e. ensuring that the handler's type is carried over to the calling point, which is already taken care of.

[fregante]

(I'll comment in review threads so that the conversation is easier to follow)

If you write as a constant you get a bit better error message

It's saying that the type is wrong because if: 1) I have something of type message, 2) I can't substitute sumMethod for it because the args might not be numbers

Indeed, as shown by your example:

async function inner(
  _: browser.runtime.MessageSender,
  ...addends: number[]
): Promise<number> {
	return addends.reduce((a, b) => a + b);
};
const outer: Method = inner;

This essentially tells tsc to discard any types of inner and just set outer to Method, so outer has no number parameters when called.

In practice this isn't a problem because outer is called with whatever onMessage receives.

What's important is that:

• inner (the "method") has the right signature for meta, which it does (this is enforced by addHandler’s types)

• outer (the "public method") has the right interface, because that's what the user calls, and that works even if I specify any[], as you suggested 🎉

  

[fregante]

The publicMethod/method fields are only for type enforcement, they're not meant to be used by users of the library. You can see them in index.ts and that's the only place they'll ever appear. Names can be improved.

The difference in types is to hide the implementation details from the caller, which appears as a regular function, this is publicMethod's signature:

• the caller calls foo(a, b, c)

While the handler must also receive meta information, which is defined by the method signature:

• the handler will have the signature foo(meta, a, b, c)

The previous POC had a similar pattern, I think:

foo([a, b, c]) // caller, “public method”
foo([a, b, c], meta) // handler, “method”

I chose to avoid this array-based signature because it's more awkward to type, as a user of the library:

function foo(meta: Meta, a: string, b: number, c: Map) {}
// versus
function foo([a, b, c]: [string, number, Map]) {}

I just realized though that most lifters currently in the extension do not have access to this meta at all, only the dev tools have a target/port. I only added it here because the POC had it.

I can probably replace it with this though, 🤔 this would hide it from most handlers:

// Regular handler, completely standard function
function foo(a: string, b: number, c: Map) {}

// Handler that needs the meta
function fooWithThis(this: Meta, a: string, b: number, c: Map) {
  if (this.immediateSender.url) {
    return true;
  }
}

Both functions have the same signature in practice since this is optional.

[twschiller]

The publicMethod/method fields are only for type enforcement

Thanks for the explanation. I think I was confused about a comment somewhere saying it was concrete

I just realized though that most lifters currently in the extension do not have access to this meta at all

And that was a mistake. As a result there's places in the extension code I had to fall back to using raw browser message handling. The point of this messaging project is to unify how we're messaging

I can probably replace it with this though

I'm not following that comment - is this referring to a class instance somehow?

[fregante]

This is ready to be merged for me

[fregante]

I just tested this locally and it appears that it can be used by any context towards the background. So I should be able to replace most liftBackground calls in the extension (still gradually though)