Skip to content
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

Messaging architecture #1

Closed
fregante opened this issue Jun 22, 2021 · 2 comments
Closed

Messaging architecture #1

fregante opened this issue Jun 22, 2021 · 2 comments
Assignees

Comments

@fregante
Copy link
Collaborator

I like the current setup for a few reasons:

  • the caller can call a lifted function whether it's local or external
  • the functions appear as regular async functions and they're typed correctly
  • it's somewhat guaranteed that the internal message is being listened to (compared to "is there a handler for this message on the other side or did we forget to register it?")

However there are some issues:

  • dependencies are bundled whether they're used locally or not
  • because they're opaque, any async function may or may not throw messaging errors, which may or may not need to be handled in the calling point
  • double-hop functions need to be explicitly lifted twice (a lot of duplicate code)

We can discuss this further to see whether these issues exist. I can also look into improving messaging without losing the good parts.

I mentioned I had already thought about a possible (explicit) messaging architecture, but this did not carry over the function types.

webext-messenger example
// unsafe.js
import {sendMessage} from './messenger';
const key = await sendMessage('get-key', otherArg);
// contentscript.js
import './messenger'; // Proxies automatically
// background.js
import {onMessage} from './messenger';

onMessage({
  name: 'get-key',
  allowUnsafe: true,
  async handler(event) {
    if (event.source === 'unsafe') {
      return getKey(...event.args);
    }
  }
});

Also I find the code sharing with PB.com problematic because that introduces another invisible step that does not guarantee at all that the shared functions have the same signature. I found that Chrome occasionally does not update the extension until you completely remove it and reinstall it. Refined GitHub users are occasionally a few months behind for no apparent reason. We can discuss why this was implemented and if there's any alternative to it.

@twschiller
Copy link
Collaborator

twschiller commented Jun 25, 2021

Thanks for looking into this! Overall, I want to keep this issue in the back of our minds, but focus on other things first (error reporting, testing infrastructure), as that will inform

Additional Considerations

  • Notifications (messages that are fire and forget)
  • Broadcasts (messages to all tabs, and potentially using the result)
  • Leveraging persistent communication channels to reduce latency (e.g., we currently use ports between the devtools and background page)
  • In the React layer, we'll want to potentially wrap some of the messages in Observable so that unsolicited messages can be reacted to. E.g., showing context script errors in the dev tools if the user has them open
  • Target of the message needs to include both tab and frame
  • MV3: there's currently an issue with dropped messages when the background worker get recycled: clarification request: what happens when a MV3 SW is shut down before answering a message w3c/webextensions#16

Throughout the code, you'll see places where the original design didn't support all the cases, so I had to introduce new versions of "lift", or not use the "lift" approach entirely and fall back to switch statements

I suspect we'll need to turn the background page into a message bus/router. The other components might need to have a message queue to handle retries/resends/etc.

Responses

I mentioned I had already thought about a possible (explicit) messaging architecture

We should also look into other prior art from major OSS extensions to see if there's any ideas we can grab. For example:

dependencies are bundled whether they're used locally or not

The way to remove this coupling is most likely to share a TypeScript types (i.e., the Protocol), but not any implementation code. This could ensure type safety and that all messages are handled

double-hop functions need to be explicitly lifted twice

I think there might even be some triple hop ones of Dev Tools -> Background Page -> Content Script -> Page Script

any async function may or may not throw messaging errors

Could you give an example of this? I'm not sure I follow

invisible step that does not guarantee

It becomes an API evolution problem, that analogous to what you've have on any client + server architecture. The API has to evolve in a way that's backward compatible

From telemetry, we know which extension versions are still deployed. So can use that information to help know when we can make backward incompatible changes

I found that Chrome occasionally does not update the extension until you completely remove it and reinstall it.

I can't find the documentation link, but I believe this is the case when the background page is set as persistent (which is the default). Chrome will download the extension in the background, but won't install until you restart Chrome

For helping users stay up-to-date, the options are:

  • On app.pixiebrix, have a stable external messaging API to check the version of the extension so it know any restrictions in talking to a legacy extension version
  • On app.pixiebrix, have a stable API for triggering the extension update from the app.pixiebrix app
  • The extension is allowed to check for updates from the CWS a few times each day: https://developer.chrome.com/docs/extensions/reference/runtime/#method-requestUpdateCheck
  • On app.pixiebrix, we can expose an API that has the CWS version, and the extension could ping that on a more regular basis. For more granularity, we could introduce a "minimum version" key in blueprints/bricks, so the extension knows it needs to look for an update in order to use a particular blueprint/brick

twschiller referenced this issue in pixiebrix/pixiebrix-extension Aug 14, 2021
twschiller referenced this issue in pixiebrix/pixiebrix-extension Aug 14, 2021
@fregante fregante self-assigned this Aug 20, 2021
@fregante fregante transferred this issue from pixiebrix/pixiebrix-extension Aug 21, 2021
@fregante
Copy link
Collaborator Author

fregante commented Sep 2, 2021

We discussed about the various routes in a different thread, so I'll link it here for future reference:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants