-
Notifications
You must be signed in to change notification settings - Fork 183
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
feat: type-safe messaging api #899
base: main
Are you sure you want to change the base?
Conversation
❌ Deploy Preview for creative-fairy-df92c4 failed.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #899 +/- ##
==========================================
- Coverage 82.29% 81.13% -1.17%
==========================================
Files 124 127 +3
Lines 6513 6642 +129
Branches 1094 1104 +10
==========================================
+ Hits 5360 5389 +29
- Misses 1139 1238 +99
- Partials 14 15 +1 ☔ View full report in Codecov by Sentry. |
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.
- How would webpage messaging be done type safe? As for example my website would connect to it but I obviously can't just import wxt from there?
We should just allow importing the messaging API there! If it's a separate package (see other comment), it's much easier to add to a web project without the entire framework. And if we don't use the polyfill, there's no harm in a website importing it!
Then ideally, they would copy the interfaces/types or reference the original types if it's in the same repo.
I think this is different from what you're thinking, you are exporting the proxy in the PR's readme. I'm thinking we don't import the proxy directly in the project, but call the API from the messaging library to create a proxy in the web project.
// apps/web/extension-proxy.ts
import { createRpcProxy } from '@wxt-dev/messaging/rpc'
import type { SomeService } from 'apps/extension/utils/...'
export const someService = createRpcProxy<SomeService>(...)
- How would native messaging work in a type safe way, probs same question as 1.
This one is a hard one. I have a safari extension at work, and I wrote a script that looks at the TypeScript AST for the messaging protocol, and generates iOS swift code to match. A generic approach for this... probably isn't possible.
But for Electron, which uses JS, the messaging library could also just be imported there. If it's in a monorepo, they could inherit types from the extension's package.
But yeah, TLDR, there's probably not a generic solution for this.
- What is an example for an injected iframe?
Send a message from a content script to an iframe created with createIframeUi
. I think I have a simple solution for this, a transport that uses window.postMessage
, keep reading...
- How would I implement tests into this, could you maybe help with that if possible?
Also covered in the next section, but yes, I will help with testing.
General Questions
- Should class properties be accessible on the other side too? (This would mean they'd be "streamed" to the other side but could only support basic types) - I don't think this should be done
Nope, stripping them out of the types like you did is fine... The other option would be to convert it into an async function and return the value that way? Doesn't seem super useful to me.
Alright, I like where this is headed, but there's one thing that stands out to me. Seems like you're struggling on how to incorporate the different types of messaging (ports vs one time requests vs native etc). Adding flags to "enable" each method is one approach, but I don't think it scales well. The number of if-statements will scale exponentially in this case.
This is something I also struggled with in @webext-core/messaging
, and my solution is a spaghetti nightmare lol. Every time I look at that code, I cringe.
So after I saw some of your messages from the last few days, I spent a long time today thinking about how to handle this problem.
Let's consider at the RPC API as an example. Right now, your code supports proxy-ing some service in the background to a HTML page or content script. What if someone wants to proxy the extension API from a content script to a main world content script? Or proxy native code to the background? Or worst case, they want to call native code inside a main world content script lol. These are things that are missing from @webext-core/proxy-service
, your code, and other RPC like libraries trying to accomplish the same thing.
We basically need to pass in a custom communication layer that the RPC APIs will use to send messages. Then the user could choose which comunication method to use, and we don't have any if-statements in our code.
I just pushed a new folder to this branch. It re-imagines what an RPC library would look like based on this concept of communication layers.
https://github.com/Timeraa/wxt/tree/wxt-messaging/packages/messaging-aaron
Main thing to look at is the MessageTransport
interface. Each communication method (native, one-time message, ports, window, external, custom events, user scripts, etc) will have an implementation for this interface, and thus can be hot-swapped when necessary.
Each transport implementation is responsible for sending and receiving messages, as well as handling errors.
At this point, I've done 3 things:
- Define the
MessageTransport
interface - Implement an example RPC module based on the transports interface and supports all the use-cases I mentioned above
- Implement a unit test friendly version of the
MessageTransport
to test the RPC module
Note: I have not implemented any of the real transports using the extension API, like ports or one-time messages or native yet. Because with the interface, I didn't need to implement them to know that the RPC module will work - the testing implementation is enough. As for writing tests for the other transports... I can help with that.
I'd love to have your thoughts on this. I didn't talk about the other two APIs (distributed and event-based), but I designed the transport interrface to work for both of those as well. This post will be 3x longer if I go into that detail now...
packages/wxt/src/util/index.ts
Outdated
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 added a wxt/util export for a useful function imo, is that okay to keep there?
Add utils like this to wxt/client
if it uses the extension API or wxt/sandbox
if it doesn't. For example, the createShadowRootUi
is imported from wxt/client
, while the MatchPattern
class is imported from wxt/sandbox
.
This was originally done so the polyfill isn't ran anywhere that doesn't support it. However, with the move away from the polyfill, I'll likely simplify this setup and export everything from wxt/client
in the future.
But I digress, this is a great function! Could also be as simple as checking if location.pathname
contains "background"
somehow...
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 I had some issues with just checking location.pathname in other browsers, would need to test that in depth at some point, thanks for the heads up on the first part!
packages/wxt/src/rpc/index.ts
Outdated
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 want the messaging APIs to be a separate NPM pacakge, I think it would be neat to make them available to everyone to use, not just to people using WXT.
But I also maybe want to include them in WXT by default. In that case, this file would become:
export * from '@wxt-dev/messaging/rpc';
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.
Okay, abstracting it so we can make it a different module defo solves a lot of issues mentioned above, I'll rework it.
I like your abstraction approach, should've thought of that earlier tbh! I'll implement it the way you suggested it, I don't see any issues with it.
|
in the MessageTransport I added a |
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.
The only thing is that if you want to send something from the injected frame > background you'd need an in between proxy but I think that should be fine and makes it logically followable (Which is really good especially because extensions can be a huge brainfuck with messaging in different contexts), just needs to be documented.I think you already did that with the message bridge, I'm half asleep already
Message bridge is separate, it's meant for the "decentralized" approach, though maybe it could be used by the RPC api. I was thinking you should chain the proxies together:
// background
const myService = createRpcProxy<MyService>("my-service", createNativeTransport());
registerRpcService(myService);
// content script
const myService = createRpcProxy<MyService>("my-service"); // Use ports to send to background
registerRpcService(myService);
// main world
const myService = createRpcProxy<MyService>("my-service", createExternalMessageTransport())
myService.xyz();
But maybe something like this would be much simpler...
// background
bridge.init(); // or just import the object created by `createMessageBridge()`?
// content script
bridge.init(); // or just import the object created by `createMessageBridge()`?
// main world
const myService = createRpcProxy<MyService>("my-service", { target: "native" });
myService.xyz();
What would the sentAt be used for btw?
Just debugging purposes. Could probably be removed. I figured it would be a nice-to-have for some kind of devtools UI eventually.
This reverts commit d302fc3.
Caution
Implementation and code are not done at all, feel free to test and provide feedback about changes though but note that code can change anytime
Note
Status: POC (Proof of Concept)
This is a tracking PR which aims to implement #643
Internal Messaging (via ports aka chrome.runtime.connect)
Native Messaging (via ports aka chrome.runtime.connectNative)
Internal Messaging (via sendMessage)
Native Messaging (via sendNativeMessage)
UserScript Messaging (via ports)
Chromium mv2- Doesn't existSafari mv2- Doesn't exist?Injected script in root page Messaging
General Todos
I am currently focusing on implementing the Background as an API approach, after this is done I will try to implement others if wanted by @aklinker1.
Questions for @aklinker1
General Questions
Usage
Check the package README