Use Messenger in more content script places (Native Editor, Dev Tools) #1460

fregante · 2021-09-24T18:34:54Z

Intent

Remove most usage of liftContentScript as part of: Prevent context-specific code from being imported in the wrong context #669
Add notification support webext-messenger#24 building on Replace notifyContentScripts with Messenger #1329

Context

Many of these handlers are just forwarders (which haven't been implemented yet), but making this change first prepares the content script API (it will be final) without having to test forwarding on top of that.

Past Messenger PRs

fregante

Diff best seen without whitespace changes

fregante · 2021-09-24T18:42:35Z

src/common.ts

-  DETECT_FRAMEWORK_VERSIONS
-);
+export const withDetectFrameworkVersions = createSendScriptMessage<
+  FrameworkMeta[]


Note: This type was changed because withDetectFrameworkVersions had an assertion to FrameworkMeta in the only place where it was used.

fregante · 2021-09-25T19:09:24Z

src/background/deployment.ts

-      queueReactivateTab,
-      "Reactivation queue failed for some tabs"
-    );
+    void forEachTab(queueReactivateTab);


Since the messenger now supports notifications natively (pixiebrix/webext-messenger#24), notifyTabs was replaced by a simple loop + call of the method, which means the errors are no longer logged together.

fregante · 2021-09-25T19:13:46Z

src/contentScript.ts

-import "@/contentScript/devTools";
-import "@/contentScript/contextMenus";
 import addContentScriptListener from "@/contentScript/backgroundProtocol";
 import { handleNavigate } from "@/contentScript/lifecycle";
 import addExecutorListener from "@/contentScript/executor";
 import "@/messaging/external";
-import "@/contentScript/script";
 import "@/vendors/notify";
 import { markReady, updateTabInfo } from "@/contentScript/context";
 import { initTelemetry } from "@/telemetry/events";
-import "@/contentScript/uipath";


All the unassigned imports removed by this PR were only used to register the listeners and have been double-checked for other side-effects (but not their sub-dependencies)

fregante · 2021-09-25T19:15:27Z

src/contentScript/devTools.ts

 import { BlockConfig } from "@/blocks/types";
 import { cloneDeep } from "lodash";
 import ConsoleLogger from "@/tests/ConsoleLogger";
 import { SerializableResponse } from "@/messaging/protocol";

-if (isContentScript()) {
-  addListenerForUpdateSelectedElement();


This should be the only "active" code and it's been moved to @/contentScript.ts

fregante · 2021-09-25T19:16:10Z

src/contentScript/lifecycle.ts

-  if (openerTabId != null) {
-    console.debug(`Setting opener tabId: ${openerTabId}`);
-  }


Unused, removed

fregante · 2021-09-25T19:17:00Z

src/contentScript/messenger/api.ts

-  "QUEUE_REACTIVATE_TAB"
+  "QUEUE_REACTIVATE_TAB",
+  { isNotification: true }
 );
-export const reactivateTab = getContentScriptMethod("REACTIVATE_TAB");
+export const reactivateTab = getContentScriptMethod("REACTIVATE_TAB", {
+  isNotification: true,
+});


These methods were added in #1329 but now they're real "notifications" 🎉

fregante · 2021-09-25T19:22:19Z

src/nativeEditor/dynamic.ts

+export async function updateDynamicElement({
+  extensionPoint: extensionPointConfig,
+  extension: extensionConfig,
+}: DynamicDefinition): Promise<void> {


Where possible, I also added return values for https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/explicit-module-boundary-types.md

The rule is useful because it ensures we don't change a function’s signature by mistake.

fregante · 2021-09-25T19:25:45Z

src/contentScript/messenger/registration.ts

+  INSTALLED_EXTENSIONS: getInstalledIds,
+  CHECK_AVAILABLE: checkAvailable,
+  HANDLE_NAVIGATE: handleNavigate,
+  SHOW_NOTIFICATION: showNotification,


Test report

Seen in the console

REACTIVATE_TAB

RUN_SINGLE_BLOCK

READ_SELECTED

DETECT_FRAMEWORKS

CLEAR_DYNAMIC_ELEMENTS - seen in console but I'm not sure I saw it working

UPDATE_DYNAMIC_ELEMENT

ENABLE_OVERLAY - I don't know what this does 🤔

DISABLE_OVERLAY

INSTALLED_EXTENSIONS

CHECK_AVAILABLE

HANDLE_NAVIGATE

SHOW_NOTIFICATION

RUN_READER

Unable to test

INSERT_PANEL - behind beta flag?

INSERT_BUTTON - behind beta flag?

UIPATH_INIT - I'm not sure whether I can use this

UIPATH_GET_PROCESSES - Same

SEARCH_WINDOW - Unused. It appears in devTools/Locator but that file isn't used anywhere.

RUN_READER_BLOCK - unused

The beta flag you can always fake by hard-coding the check for the beta flag

INSERT_BUTTON: this is what is used when adding a button the add Menu Item / Button flow in the page editor

I'll add the UiPath ones to the testing list for 1.4.0. They have to be tested on Windows with a UiPath local install

ENABLE_OVERLAY is used to show an overlay over an element you have selected when using SelectorSelectorWidget

twschiller · 2021-09-26T23:13:11Z

src/background/contextMenus.ts

-      message: "Ran content menu item action",
-      className: "success",
-    });
+    void showNotification(target, "Ran content menu item action", "success");


I'd prefer not to change the API of the showNotification method. Having multiple arguments of the same type is error prone (if the arguments are swapped, TypeScript won't complain). Also, using the object will allow us to expose more options in the future

Could we go back to the object-based method signature? Alternatively, we could have the message as an argument and then put className in an options object parameter

Avoid error-prone method signature

multiple arguments of the same type is error prone

Agreed, but this is not the case, className has a union type:

pixiebrix-extension/src/contentScript/notify.ts

Lines 20 to 23 in d6890dc

export async function showNotification(

message: string,

className: "error" | "info" | "success"

): Promise<void> {

I reverted the change anyway though

argh sorry the commit push had failed somehow and I merged without the change. I included the change in the next PR https://github.com/pixiebrix/pixiebrix-extension/pull/1477/files#diff-660a48b387ef0f402e92201cc396679f79e0a52a43b6e6960f5ff07ef756d5b3

twschiller

@fregante see comment about the showNotification API

Otherwise, let's get this merged in so we'll get some days of testing amongst the dev team before the 1.4.0 submission

fregante added 2 commits September 25, 2021 19:02

Use Messenger in more places (Native Editor, Dev Tools)

97167e5

Reduce console noise

985d453

fregante force-pushed the F/messenger/more branch from 628d29e to 985d453 Compare September 25, 2021 12:05

fregante mentioned this pull request Sep 25, 2021

Add notification support pixiebrix/webext-messenger#24

Merged

2 tasks

Add notifications

4e56b1c

fregante commented Sep 25, 2021

View reviewed changes

fregante changed the title ~~WIP: Use Messenger in more places (Native Editor, Dev Tools)~~ Use Messenger in more content script places (Native Editor, Dev Tools) Sep 25, 2021

Fix Jest 🤷‍♂️

9df42d5

fregante added the performance label Sep 25, 2021

Lint 😌

d6890dc

fregante marked this pull request as ready for review September 25, 2021 19:45

fregante mentioned this pull request Sep 26, 2021

Use Messenger for CS block executor + NativeEditor selector #1477

Merged

3 tasks

twschiller reviewed Sep 26, 2021

View reviewed changes

twschiller approved these changes Sep 26, 2021

View reviewed changes

fregante merged commit 5937ad2 into main Sep 27, 2021

fregante deleted the F/messenger/more branch September 27, 2021 02:55

fregante mentioned this pull request Oct 2, 2021

Use Messenger for all forwarded devtools/CS functions #1546

Merged

fregante mentioned this pull request Oct 12, 2021

Use Messenger for Registry functions #1630

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Messenger in more content script places (Native Editor, Dev Tools) #1460

Use Messenger in more content script places (Native Editor, Dev Tools) #1460

fregante commented Sep 24, 2021 •

edited

Loading

fregante left a comment

fregante Sep 24, 2021

fregante Sep 25, 2021 •

edited

Loading

fregante Sep 25, 2021 •

edited

Loading

fregante Sep 25, 2021

fregante Sep 25, 2021

fregante Sep 25, 2021 •

edited

Loading

fregante Sep 25, 2021 •

edited

Loading

fregante Sep 25, 2021 •

edited

Loading

twschiller Sep 26, 2021

twschiller Sep 26, 2021

twschiller Sep 26, 2021 •

edited by fregante

Loading

fregante Sep 27, 2021

fregante Sep 27, 2021

twschiller left a comment •

edited

Loading

	export async function showNotification(
	message: string,
	className: "error" \| "info" \| "success"
	): Promise<void> {

Use Messenger in more content script places (Native Editor, Dev Tools) #1460

Use Messenger in more content script places (Native Editor, Dev Tools) #1460

Conversation

fregante commented Sep 24, 2021 • edited Loading

Intent

Context

Next

Past Messenger PRs

fregante left a comment

Choose a reason for hiding this comment

fregante Sep 24, 2021

Choose a reason for hiding this comment

fregante Sep 25, 2021 • edited Loading

Choose a reason for hiding this comment

fregante Sep 25, 2021 • edited Loading

Choose a reason for hiding this comment

fregante Sep 25, 2021

Choose a reason for hiding this comment

fregante Sep 25, 2021

Choose a reason for hiding this comment

fregante Sep 25, 2021 • edited Loading

Choose a reason for hiding this comment

fregante Sep 25, 2021 • edited Loading

Choose a reason for hiding this comment

fregante Sep 25, 2021 • edited Loading

Choose a reason for hiding this comment

Test report

Seen in the console

Unable to test

twschiller Sep 26, 2021

Choose a reason for hiding this comment

twschiller Sep 26, 2021

Choose a reason for hiding this comment

twschiller Sep 26, 2021 • edited by fregante Loading

Choose a reason for hiding this comment

fregante Sep 27, 2021

Choose a reason for hiding this comment

fregante Sep 27, 2021

Choose a reason for hiding this comment

twschiller left a comment • edited Loading

Choose a reason for hiding this comment

fregante commented Sep 24, 2021 •

edited

Loading

fregante Sep 25, 2021 •

edited

Loading

fregante Sep 25, 2021 •

edited

Loading

fregante Sep 25, 2021 •

edited

Loading

fregante Sep 25, 2021 •

edited

Loading

fregante Sep 25, 2021 •

edited

Loading

twschiller Sep 26, 2021 •

edited by fregante

Loading

twschiller left a comment •

edited

Loading