#4258: Remove trigger listeners after invalidation #7675

fregante · 2024-02-20T12:30:30Z

What does this PR do?

Part of Cleanup widgets and side effects on context invalidated (reload, update) #4258
Uses and extends SimpleEventTarget to simplify removal via AbortSignal, mentioned in https://github.com/pixiebrix/pixiebrix-extension/pull/7628/files#diff-65845b53c0bee092173f49144dcc9aff4219e6f7a707295a3db7a704e4a7dad0R47

Demo

wip, there's an existing behavior issue to solve first

Checklist

Designate a primary reviewer: @mnholtz

src/starterBricks/sidebarExtension.ts

fregante · 2024-02-20T12:42:31Z

src/starterBricks/triggerExtension.ts

+      id: this.id,
+      instanceNonce: this.instanceNonce,
+    });
+


The test for this file is failing on watch because:

before: the events would be removed in the uninstall function

now: the events are removed in cancelObservers (via abortcontroller)

The difference is that cancelObservers is being called by runModComponents. Is that supposed to happen?

pixiebrix-extension/src/starterBricks/triggerExtension.test.ts

Lines 242 to 249 in 0ffbc92

await extensionPoint.install();

await extensionPoint.runModComponents({ reason: RunReason.MANUAL });

expect(rootReader.readCount).toBe(0);

screen.getByRole("button").click();

await tick();

expect(rootReader.readCount).toBe(1);

Are the tests calling the wrong function? Why is runModComponents calling cancelObservers even on watch?

pixiebrix-extension/src/starterBricks/triggerExtension.ts

Lines 799 to 800 in 0ffbc92

async runModComponents(): Promise<void> {

this.cancelObservers();

cc @twschiller because both snippets were written by you, you might have more context.

I think the trigger extension point might need 2 layers of abort controllers?

Overall abort controller

Abort controller for the mutation observer/intersection observer(s) added in the appear trigger and "watch". (Or maybe just the "watch" mutation observers

Why is runModComponents calling cancelObservers

On SPA navigation, lifecycle calls runModComponents. The cancelObservers call aborts the observer/intersection observer so they can be re-added

I think it might be valid just to keep those registered on SPA navigation if they're already registered. But you'd definitely need logic to avoid re-adding the mutation/intersection observer. I don't remember exactly, but I probably just took the route of removing/re-adding vs. keeping track of state because

Also, the removing/re-adding would be resilient to changes in the trigger selector. (Although the trigger is readonly for trigger instances. If the trigger changes via the Page Editor IIRC the old extension point gets uninstalled)

Corner cases to consider:

What happens on SPA navigation to a URL that's not targeted by the trigger - if this is not already handled, it should be handled by lifecycle.ts calling uninstall:

pixiebrix-extension/src/contentScript/lifecycle.ts

Line 120 in 4119b67

if (!(await extensionPoint.isAvailable())) {

What happens on Page Editor edit - IIRC, this already works because uninstall is called on the extension point?

I think I'll do with the dual controller to replicate the previous behavior. I wouldn't want to change the logic further in this PR.

fregante · 2024-02-20T12:51:33Z

src/starterBricks/triggerExtension.ts

-      $elements.on(domEventName, this.eventHandler);
+      for (const element of $elements) {
+        element.addEventListener(domEventName, this.eventHandler, {
+          signal: this.abortController.signal,


The signal here is what's failing the tests. If I keep the events until uninstall is called, it doesn't fail, but then what is cancelObserver for?

it doesn't fail, but then what is cancelObserver for?

It's specifically for the mutation/intersection observers: #7675 (comment)

fregante · 2024-02-20T12:51:37Z

src/starterBricks/triggerExtension.ts

@@ -768,17 +746,22 @@ export abstract class TriggerStarterBrickABC extends StarterBrickABC<TriggerConf
      $elements.off("mouseenter.hoverIntent");
      $elements.off("mouseleave.hoverIntent");
      $elements.hoverIntent({
-        over: this.eventHandler,
+        over: async ({ originalEvent }) => this.eventHandler(originalEvent),


The previous piece of code used event.originalEvent everywhere. This simplifies it by calling it once.

fregante · 2024-02-20T12:53:26Z

src/store/enterprise/managedStorage.ts

-    initializationInterval = undefined;
-  }
+  clearInterval(initializationInterval);
+  initializationInterval = undefined;


No need to do a null-check, as long as the type isn't null (which isn't)

fregante · 2024-02-20T12:53:54Z

src/utils/SimpleEventTarget.ts

-    this.addEventListener(this.coreEvent, this.getNativeListener(callback));
+  add(
+    callback: SimpleEventListener<Detail>,
+    options?: AddEventListenerOptions,


Added support for {signal} etc

twschiller · 2024-02-20T13:04:52Z

src/store/enterprise/managedStorage.ts

@@ -35,30 +36,17 @@ const MAX_MANAGED_STORAGE_WAIT_MILLIS = 4500;
 /**
 * Interval for checking managed storage initialization that takes longer than MAX_MANAGED_STORAGE_WAIT_MILLIS seconds.
 */
-let initializationInterval: Nullishable<ReturnType<typeof setTimeout>>;
+let initializationInterval: ReturnType<typeof setTimeout> | undefined;


I'd recommend calling out that it should be undefined instead of Nullishable because of clearInterval has different behavior on null vs. undefined: https://github.com/pixiebrix/pixiebrix-extension/pull/7675/files#r1495773879

I can do it, but:

we shouldn't really prefer Nullishable over the single types (X | null OR X | undefined), but only if the type is necessarily X | null | undefined

it's not that clearInterval has a different behavior, but the types don't allow null, so the change wouldn't pass the types checker

So as long as we don't go around replacing | undefined with Nullishable, this isn't an issue (and even then, tsc will immediately complain)

we shouldn't really prefer Nullishable over the single types (X | null OR X | undefined), but only if the type is necessarily X | null | undefined

@grahamlangford to avoid future bike-shedding I recommend we decide an approach and enforce via eslint

I agree with @fregante that's unnecessarily expanding types isn't valuable. The counter principle for our own code is that we by and large shouldn't be writing code where the distinction between null vs. undefined matters

twschiller · 2024-02-20T13:07:07Z

src/store/enterprise/managedStorage.ts

-  }
-}
+// Used only for testing
+let controller = new AbortController();


Where does the abort controller get used? I'm not seeing it get passed to the event target or interval?

It's being used in INTERNAL_reset? Federico's comment mentions its only used for testing. Or maybe you reviewed this before he added that comment?

mnholtz · 2024-02-20T16:55:34Z

src/starterBricks/sidebarExtension.ts

@@ -342,10 +330,9 @@ export abstract class SidebarStarterBrickABC extends StarterBrickABC<SidebarConf

  /**
   * Shared event handler for DOM event triggers.
+   * It's bound to this instance so that it can be removed when the extension is uninstalled.


Is this what you mean in our newest terminology?

Suggested change

* It's bound to this instance so that it can be removed when the extension is uninstalled.

* It's bound to this instance so that it can be removed when the mod is deactivated.

Maybe? The file is named -Extension and the class has a uninstall method, so we should probably match those (one way or another)

Part of the continuing naming/domain modeling work is to move from installed/uninstalled -> activated/deactivated.

We haven't really discussed changing the class methods though.

I'll update the comment as Misha suggested for now because that's more likely to be left behind when the methods change.

fregante

Ready

fregante · 2024-02-22T16:06:28Z

src/analysis/ReduxAnalysisManager.ts


      // Capture state at the moment of the action
      const state = listenerApi.getState();

-      abortController = new AbortController();


I noticed that we have this pattern of creating, aborting, and recreating abort controllers. I thought it could be wrapped up and clarified in a single pattern:

https://github.com/fregante/abort-utils/blob/main/source/repeatable-abort-controller.md

Thoughts on naming welcome.

Thoughts on naming welcome.

I think the naming is good enough. I'm not sure how generally applicable it will be, but worth refactoring

fregante · 2024-02-22T16:09:51Z

src/starterBricks/menuItemExtension.ts

@@ -255,7 +255,6 @@ export abstract class MenuItemStarterBrickABC extends StarterBrickABC<MenuItemSt
    super(metadata, logger);
    this.menus = new Map<UUID, HTMLElement>();
    this.removed = new Set<UUID>();
-    this.cancelController = new AbortController();


By the way we should move all of these initializations to the class itself. This is now possible:

class { map = new Map() }

instead of:

class { map: Map; constructor() { this.map = new Map(); } }

fregante · 2024-02-22T16:11:56Z

src/starterBricks/triggerExtension.ts

   */
-  private abortController = new AbortController();
+  private readonly cancelHandlers = new RepeatableAbortController();


This is the dual-controller setup I mentioned, which fixes the tests

codecov · 2024-02-22T16:22:15Z

Codecov Report

Attention: Patch coverage is 86.36364% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 72.57%. Comparing base (a8d3c3b) to head (b207c5b).
Report is 2 commits behind head on main.

Files	Patch %	Lines
src/starterBricks/sidebarExtension.ts	66.66%	4 Missing ⚠️
src/starterBricks/triggerExtension.ts	88.23%	2 Missing ⚠️
src/store/enterprise/managedStorage.ts	83.33%	2 Missing ⚠️
src/starterBricks/panelExtension.ts	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7675      +/-   ##
==========================================
+ Coverage   72.54%   72.57%   +0.02%     
==========================================
  Files        1259     1259              
  Lines       39264    39244      -20     
  Branches     7326     7321       -5     
==========================================
- Hits        28484    28480       -4     
+ Misses      10780    10764      -16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-02-22T22:42:36Z

No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack.

Do not edit this comment manually.

fregante added 2 commits February 20, 2024 18:56

Remove trigger listeners after invalidation

0ffbc92

Revert unrelated change

4119b67

fregante commented Feb 20, 2024

View reviewed changes

vercel bot deployed to Preview February 20, 2024 12:56 View deployment

fregante added the bug Something isn't working label Feb 20, 2024

fregante requested a review from mnholtz February 20, 2024 12:56

fregante assigned fregante and mnholtz Feb 20, 2024

fregante marked this pull request as ready for review February 20, 2024 12:57

twschiller requested review from grahamlangford and twschiller February 20, 2024 13:00

twschiller reviewed Feb 20, 2024

View reviewed changes

mnholtz reviewed Feb 20, 2024

View reviewed changes

fregante added 3 commits February 22, 2024 23:46

Add dual controller; use RepeatableAbortController

1de0d0c

Use in other places

021dbb9

Merge remote-tracking branch 'origin/main' into F/bug/remove-triggers

fadcf6d

vercel bot deployed to Preview February 22, 2024 15:49 View deployment

Add missing import

c1e7700

vercel bot deployed to Preview February 22, 2024 16:00 View deployment

Lint

adf69f6

fregante commented Feb 22, 2024

View reviewed changes

vercel bot deployed to Preview February 22, 2024 16:15 View deployment

Update "extension is uninstalled" comment

b207c5b

vercel bot deployed to Preview February 22, 2024 17:10 View deployment

twschiller approved these changes Feb 22, 2024

View reviewed changes

twschiller added this to the 1.8.10 milestone Feb 22, 2024

twschiller added the developer experience label Feb 22, 2024

twschiller merged commit 38df206 into main Feb 22, 2024
26 checks passed

twschiller deleted the F/bug/remove-triggers branch February 22, 2024 22:42

fregante mentioned this pull request Feb 29, 2024

#7675: Reuse SimpleEventTarget wherever possible #7765

Merged

2 tasks

twschiller pushed a commit that referenced this pull request Feb 29, 2024

#7675: Reuse SimpleEventTarget wherever possible (#7765)

cd2e37a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#4258: Remove trigger listeners after invalidation #7675

#4258: Remove trigger listeners after invalidation #7675

fregante commented Feb 20, 2024 •

edited

Loading

fregante Feb 20, 2024 •

edited

Loading

fregante Feb 20, 2024

twschiller Feb 20, 2024 •

edited

Loading

fregante Feb 20, 2024

fregante Feb 20, 2024

twschiller Feb 20, 2024

fregante Feb 20, 2024

fregante Feb 20, 2024

fregante Feb 20, 2024

twschiller Feb 20, 2024

fregante Feb 22, 2024 •

edited

Loading

twschiller Feb 22, 2024 •

edited

Loading

twschiller Feb 20, 2024 •

edited

Loading

mnholtz Feb 20, 2024

mnholtz Feb 20, 2024

fregante Feb 22, 2024

grahamlangford Feb 22, 2024

fregante Feb 22, 2024 •

edited

Loading

fregante left a comment

fregante Feb 22, 2024

twschiller Feb 22, 2024

fregante Feb 22, 2024

fregante Feb 22, 2024

codecov bot commented Feb 22, 2024 •

edited

Loading

github-actions bot commented Feb 22, 2024

	await extensionPoint.install();
	await extensionPoint.runModComponents({ reason: RunReason.MANUAL });

	expect(rootReader.readCount).toBe(0);

	screen.getByRole("button").click();
	await tick();
	expect(rootReader.readCount).toBe(1);

	async runModComponents(): Promise<void> {
	this.cancelObservers();

	* It's bound to this instance so that it can be removed when the extension is uninstalled.
	* It's bound to this instance so that it can be removed when the mod is deactivated.

#4258: Remove trigger listeners after invalidation #7675

#4258: Remove trigger listeners after invalidation #7675

Conversation

fregante commented Feb 20, 2024 • edited Loading

What does this PR do?

Demo

Checklist

fregante Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twschiller Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fregante Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

twschiller Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

twschiller Feb 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fregante Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

fregante left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 22, 2024 • edited Loading

Codecov Report

github-actions bot commented Feb 22, 2024

fregante commented Feb 20, 2024 •

edited

Loading

fregante Feb 20, 2024 •

edited

Loading

twschiller Feb 20, 2024 •

edited

Loading

fregante Feb 22, 2024 •

edited

Loading

twschiller Feb 22, 2024 •

edited

Loading

twschiller Feb 20, 2024 •

edited

Loading

fregante Feb 22, 2024 •

edited

Loading

codecov bot commented Feb 22, 2024 •

edited

Loading