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.

Sign up for GitHub

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

Jump to bottom

Use Messenger from devTools -> background communication #1936

Merged
merged 3 commits into from
Nov 21, 2021
Merged

Use Messenger from devTools -> background communication #1936

merged 3 commits into from
Nov 21, 2021

Conversation

fregante
Copy link
Contributor

@fregante fregante commented Nov 20, 2021 •
edited
Loading

After a messenger refactor+cooldown, I'm back to porting the last bits of communication to it.

Testing report

  • CHECK_TARGET_PERMISSIONS: works
  • INJECT_SCRIPT: works
  • SHOW_BROWSER_ACTION_PANEL: replaced by existing method, works
  • UNINSTALL_ACTION_PANEL_PANEL: replaced by existing method, works

fregante
fregante commented Nov 20, 2021
View reviewed changes
src/background/devtools/protocol.ts
import {
removeActionPanel,
showActionPanel,
} from "@/contentScript/messenger/api";

export const registerPort = liftBackground(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last part of the code that uses the port is emitDevTools, which thanks to pixiebrix/webext-messenger#41 it will be replaced by a plain chrome.runtime.sendMessage-based messenger.

src/background/messenger/api.ts
@@ -25,7 +25,7 @@ import browser from "webextension-polyfill";
import { isBackgroundPage } from "webext-detect-page";

// TODO: This should be a hard error, but due to unknown dependency routes, it can't be enforced yet
if (isBackgroundPage()) {
if (isBackgroundPage() && process.env.DEBUG) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This warning is not useful to users, so this should hide it from them

src/devTools/editor/hooks/useRemove.ts
@@ -65,9 +67,9 @@ function useRemove(element: FormState): () => void {
dispatch(optionsSlice.actions.removeExtension(ref));
}

void Promise.allSettled([
await Promise.allSettled([
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the behavior, I assume the intent was to await all. If not, Promise.allSettled is not needed and can be dropped

@fregante
Copy link
Contributor Author

fregante commented Nov 20, 2021 •
edited
Loading

The last liftBackground of that is currently unported is in background/protocol.ts. The issue with this part is that it automatically uses "external" communication if it's loaded from the app.

Is this intentional and necessary for all of these instances?

src/messaging/external.ts
36:const _reload = liftBackground("BACKGROUND_RELOAD", async () => {
59:const _openOptions = liftBackground("BACKGROUND_OPEN_OPTIONS", async () => {
71:const _openMarketplace = liftBackground(
106:const _openActivate = liftBackground(

src/contrib/google/bigquery/handlers.ts
54:export const readQuery = liftBackground(

src/background/installer.ts
48:export const getAvailableVersion = liftBackground(

src/background/logging.ts
204:export const recordError = liftBackground(
250:export const recordLog = liftBackground(
347:export const getLoggingConfig = liftBackground("GET_LOGGING_CONFIG", async () =>
351:export const setLoggingConfig = liftBackground(

src/background/notifications.ts
21:export const createNotification = liftBackground(

src/background/trace.ts
23:export const recordTraceEntry = liftBackground(
28:export const recordTraceExit = liftBackground(
33:export const clearExtensionTraces = liftBackground(

src/background/locator.ts
29:export const locate = liftBackground(
42:) => Promise<unknown> = liftBackground(

src/background/devtools/protocol.ts
25:export const registerPort = liftBackground(

src/background/deployment.ts
71:export const queueReactivate = liftBackground(

src/background/telemetry.ts
105:export const getDNT = liftBackground("GET_DNT", async () => _getDNT());
107:export const getUID = liftBackground("GET_UID", async () => uid());
109:export const getExtensionVersion = liftBackground(
114:export const toggleDNT = liftBackground(
161:export const initUID = liftBackground(
171:export const recordEvent = liftBackground(
193:export const sendDeploymentAlert = liftBackground(

src/background/navigation.ts
29:export const reactivate = liftBackground(

src/background/initContextMenus.ts
48:export const preloadContextMenus = liftBackground(

src/background/dataStore.ts
50:export const getRecord = liftBackground(
55:export const setRecord = liftBackground(

src/background/requests.ts
107:const backgroundRequest = liftBackground(
123:export const deleteCachedAuth = liftBackground(
240:const _proxyService = liftBackground(
280:export const clearServiceCache = liftBackground(

src/background/executor.ts
439:export const executeOnServer = liftBackground(

I think the issue with answering this question is that they might be used by components that are shared between the app and the extension. The dependency path is not clear at this point.

On a related note, I highly suggest we start placing in a dedicated folder (e.g. @/shared-app/) and block all other @/ imports in the App.

Related:

twschiller
twschiller approved these changes Nov 21, 2021
View reviewed changes
Copy link
Contributor

@twschiller twschiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will merge in so the team can kick the tires more. One pet peeve I have that's not related to this particular PR is that "thisTab" is a bit error-prone of a name since it only applies in devtools. Maybe should call it inspectedTab?

@twschiller twschiller merged commit d7a822d into main Nov 21, 2021
@twschiller twschiller deleted the F/messenger/devtool-lift-background branch November 21, 2021 17:53
@fregante
Copy link
Contributor Author

fregante commented Nov 21, 2021 •
edited
Loading

It makes sense and I will change it. However there’s always “context” related to a variable/constant. For example it’s exported by a “devtools” file and so is “getCurrentUrl”, which does not return the URL of the current page (which would be devtoolsPanel.html), but the url of the inspected tab. 😅 It also doesn’t work anywhere but the devtools, like thisTab

If you have other easy-to-fix pet peeves let me know, I always try to make code make sense.

@fregante fregante mentioned this pull request Dec 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@twschiller twschiller twschiller approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fregante @twschiller