#579: Deduplicate injection and make it more straightforward #654
Conversation
| @@ -76,7 +76,7 @@ function backgroundMessageListener( | |||
| const handlerPromise = new Promise((resolve) => { | |||
| resolve( | |||
| handler( | |||
| { tabId: meta.tabId, frameId: meta.frameId ?? 0 }, | |||
| { tabId: meta.tabId, frameId: meta.frameId ?? 0, url: meta.url }, | |||
There was a problem hiding this comment.
I wanted to pass the URL around, when available, but I'm having trouble following the flow of this lifted function so I thought it's better to ask you.
Is it possible to get the URL of the tab here? It's used to exclude pages that have permanent permissions, see the new isContentScriptRegistered function.
Currently this is undefined
There was a problem hiding this comment.
For the "lift" functions, there's always two sides to it, the calling function and the listening function. For this one, the calling side is here: http://github.com/pixiebrix/pixiebrix-extension/blob/8302a92f9ed90a4e48624a62e119e193fe2b278c/src/background/devtools/internal.ts#L184-L184
With that method setting the "meta" properties of the request here: http://github.com/pixiebrix/pixiebrix-extension/blob/8302a92f9ed90a4e48624a62e119e193fe2b278c/src/background/devtools/external.ts#L89-L89
What's interesting about that is it actually leaves off one of the expected fields: http://github.com/pixiebrix/pixiebrix-extension/blob/8302a92f9ed90a4e48624a62e119e193fe2b278c/src/background/devtools/contract.ts#L56-L56
I'm not sure if URL is available at that call site, as I don't see it on the type declaration for browser.devtools.inspectedWindow
Historic context: for the messaging protocol, I just copied the Flux Standard Actions approach used by Redux: https://github.com/redux-utilities/flux-standard-action
There was a problem hiding this comment.
I wanted to pass the URL around
I'm not sure passing the URL around simplifies things due to the additional book keeping. I guess one benefit it we can sort of enforce this: w3c/webextensions#8
There was a problem hiding this comment.
Thanks, I’ll look into it again today. The types are indeed failing me here, likely due to to an assertion in the callBackground call. These kind of functions have complex types so it’s hard/impossible to get them right. I regularly run into TS limitations when trying to type type-preserving functions
There was a problem hiding this comment.
Yeah the URL call was actually just in the temporary injection function, and I think it was available there, but as I’m working to have a “single injector” I moved it here without luck.
As for that issue, the only way to avoid it for us would be to do a URL check in the pre-contentScript we had discussed before
| * @param error | ||
| * @returns | ||
| */ | ||
| export async function isPrivatePageError(error: unknown): Promise<boolean> { |
There was a problem hiding this comment.
I don't think this should be async?
| } | ||
|
|
||
| /** Checks whether a tab has the activeTab permission to inject scripts and CSS */ | ||
| export async function isActiveTab( |
There was a problem hiding this comment.
This is a confusing method name. At first glance, it looks as if it should be whether the tab is the active one in the browser. I would recommend hasActiveTabPermission, as it's wordier, but instant to understand
Additionally, this will also work if the page has permanent permissions
How does this method relate to testPagePermissions?
There was a problem hiding this comment.
It probably replaces it. The PR is still a WIP, trying things out
| if (!isBackgroundPage()) { | ||
| throw new Error( | ||
| "injectContentScript can only be called from the background page" | ||
| ); | ||
| } | ||
|
|
||
| // Already has permanent access | ||
| if (await isContentScriptRegistered(target.url)) { | ||
| return; |
There was a problem hiding this comment.
Prefer return false so that it matches the return type
There was a problem hiding this comment.
Why isn’t this caught by TS? 🤔
Related to #579
@twschiller the code is WIP but I wanted to ask a question. Review coming in a minute