How to call disposers on result of computed property?

[brabeji]

Problem

I'm looking for an unobtrusive way to call disposers on mobx computed property value. That would be useful in multiple scenarios in my app. Minimal representative one goes like this:

class ElementSize {
  resizeObserver: ResizeObserver;
  @observable rect: DOMRect;

  constructor(element: HTMLElement) {
    this.resizeObserver = new ResizeObserver(this.handleResize);
    this.resizeObserver.observe(element);
    this.rect = element.getBoundingClientRect().toJSON() as DOMRect;
  }

  destructor() {
    this.resizeObserver.disconnect();
  }

  @action.bound private handleResize(entries: ResizeObserverEntry[]) {
    this.rect = entries[0].target.getBoundingClientRect();
  }

  @computed get width() { return this.rect.width; }
}

class ObservableRefObject<T extends HTMLElement> {
  @observable current: T;
  @action.bound handle(element: T | null) { this.current = element }
}

class Popover {
  triggerRef: ObservableRefObject<HTMLButtonElement> = new ObservableRefObject();
  
  @computed get triggerSize() {
    // this computed returns ElementSize instance for an element
    // obtained asynchronously from React ref.
    if(this.triggerRef.current) {
      // Obviously, we need to call `destructor()` of this instance
      // whenever it is recomputed and when it becomes unobserved.
      return new ElementSize(this.triggerRef.current)
    }
  }
}

Things I have tried

1. Saving the instance

This is what I'm using now but it's clumsy.

class Popover {
  triggerRef: ObservableRefObject<HTMLButtonElement> = new ObservableRefObject();
  
  currentTriggerSize: ElementSize | undefined;
  @computed get triggerSize() {
    this.currentTriggerSize?.destructor();
    if(this.triggerRef.current) {
      this.currentTriggerSize = new ElementSize(this.triggerRef.current);
      // I'm not even sure this is legal inside of a reaction
      onBecomeUnobserved(this, 'triggerSize', () => {
        this.currentTriggerSize?.destructor();
      })
      return this.currentTriggerSize;
    }
  }
}

2. Custom decorator

Wrap computed into a userland decorator. I couldn't get the implementation right.

class Popover {
  triggerRef: ObservableRefObject<HTMLButtonElement> = new ObservableRefObject();
  
  currentTriggerSize: ElementSize | undefined;
  @computedWithDestructor get triggerSize() {
    if(this.triggerRef.current) {
      return new ElementSize(this.triggerRef.current)
    }
  }
}

export function computedWithDestruction(target, propertyName, descriptor) {
	let initialized = false;
	let instance;

	const getter = descriptor.get;

	descriptor.get = function() {
		if (instance) {
			instance?.destructor?.();
		}
                // This seems to be called before `this.triggerRef`
                // is initialized, resulting in error :(
		instance = getter.call(this);
		if (!initialized) {
			initialized = true;
			onBecomeUnobserved(target, propertyName, () => {
				instance = undefined;
				instance?.destructor?.();
			});
		}
		return instance;
	};

	computed(target, propertyName, descriptor);
}

3. Use something similar to Auto dispose reactions[Issue & Solution :)] #2133 by @omerman

I guess this would work well #2133 but I don't like my classes to depend on the ComputedInstance exposing implementation details and it's also kind of verbose in the calling place. I also think that issue should be revisited as it's generously tagged "needs investigation" but closed.

Maybe there is some completely different pattern I'm missing, that would make this a non-issue.

Thanks for your help!

[urugator]

Not exactly sure what you are trying to acomplish, but can't you use ref callback to create/dispose ResizeObserver?

[brabeji]

That would work, but instead I want to declaratively derive the ElementSize instance from the element reference to keep things simple and decoupled. The ResizeObserver instance is an implementation detail.

This problem is not specific to React refs. I'd like to hide any cleanup logic of my classes into destructor method. Another example is possible GraphQLQuery class instance which could be @computed from query variables, and internally subscribe to some GraphQL cache updates. That subscription also has to be disposed of transparently by calling destructor when the computed evaluates.

[urugator]

You can hide the cleanup wherever you want - the question is whether you will call it explicitely as part of some handler (event/ref callback/effect/etc), which is completely transparent and easy to follow or whether you call it "reactively" when some other state changes, which quickly becomes tangled mess with no real benefit.
That being said:
You cannot access computed's cache (btw you shouldn't rely on the cache in the first place), so you need to keep the instance elsewhere.
By default you cannot perform state mutations during computation, so you may need to wrap destructor() with _allowStateChanges (perhaps runInAction suffice, not sure atm, give it a try)
onBecomeUnobserved(this, 'triggerSize', () => {}) should be created once in Popover's constructor (and note it returns disposer).

[brabeji]

I really appreciate your help! Let me clear some aspects a bit more:

... which is completely transparent and easy to follow

That's true, but it's also manual work prone to errors (forgetting to call destructor due to dislocation of the calls throughout the class). The idea is that mobx can help me with that because it knows about the computed value lifecycle.

... call (the destructor) "reactively" when some other state changes

I believe that's not what I'm doing. I want to call it specifically when the instance is no longer needed (as if JS had destructors, no other mobx state involved here). That is, I believe, when it's replaced by an updated value or the computed property ceases to be observed (presumably it cannot be read again until after the next computation).

Please do correct me if I'm wrong in my assumptions 🙏 I guess I'll wrestle some more with the custom decorator approach, taking into account your suggestions. Please let me leave this to sit open for a while before closing, to see if there is some input from others.

Thanks a bunch for your help!

[omerman]

Hey sorry to arrive late to the party :)

Regarding the issue with ComputedInstance(being verbose)
I don't find myself in need for this kind of behavior anymore.
From what I remember, I couldn't use it as an abstract class and extend it, the pattern I used doesn't fit.
But I do believe there is a way to use ComputedInstance as some sort of a class decorator maybe?

Please let my know how it plays out, I'd like to know if an elegant solution exists out there :)

[brabeji]

I think it might be possible to do the dependency other way around. I'd make my classes implement Destructible interface and then use your ComputedInstance as groundwork for some computed helper, which calls the destructor. The computed getter would then look similar as in your original design OR there could be a custom property decorator as you suggest. That would solve the problem for me.

Thanks a lot for the answer!

[omerman]

That sounds great,

If it's not a hassle, attach the final product here so more people can use it. ( and also Im curious to see the result ;) )

Good luck!

[urugator]

Possibility to consider

class ElementSize {
  resizeObserver: ResizeObserver;
  atom: Atom;
  @observable observed: boolean = false,
  @observable ref: ObservableRefObjec<HTMLElement>

  constructor(ref: ObservableRefObject<HTMLElement>) {
    this.ref = ref;
    this.resizeObserver = new ResizeObserver(() => this.atom.reportChanged());  

    this.atom = mobx.createAtom(
       'ElementSize',
       () => this.observed = true
       () => this.observed = false
    )   
    
    autorun(() => {
      if (this.ref.current && this.observed) {
        this.resizeObserver.observe(this.ref.current);
      } else {
        this.resizeObserver.disconnect()
      }
    })
  }

  @computed get width() { 
    this.atom.reportObserved();
    return this.ref.current.getBoundingClientRect().width;
  }
}

Had to edit it few times, hopefully I got it right ... the idea should be clear

[brabeji]

That looks fine! I'll revisit the Atom docs and meditate on this a bit. Thank you

[omerman]

@brabeji @urugator - Looks awesome! I revised it a little(I think that my version is a little bit more optimised) hope this helps.

class ElementSize {
  resizeObserver: ResizeObserver;
  atom: Atom;
  @observable observed: boolean = false,
  @observable ref: ObservableRefObjec<HTMLElement>

  constructor(ref: ObservableRefObject<HTMLElement>) {
    this.ref = ref;
    this.resizeObserver = new ResizeObserver(() => this.atom.reportChanged());  

    let autorunDisposer;
    this.atom = mobx.createAtom(
       'ElementSize',
       () => {
          autorunDisposer = autorun(() => {
           if (this.ref.current) {
              this.resizeObserver.observe(this.ref.current);
            }
          });
       },
       () => {
          autorunDisposer();
          this.resizeObserver.disconnect();
       }
    )   
  }

  @computed get width() { 
    this.atom.reportObserved();
    return this.ref.current?.getBoundingClientRect().width;
  }
}

[brabeji]

@urugator @omerman Thank you so much for helping me out and pointing me in the right direction, guys. Using Atom makes perfect sense and I think I've wrapped my head around applying it in the general case, keeping code maintainability 😌 .

I've put an example with short summary here https://codesandbox.io/s/zen-jennings-64ro6?file=/src/App.tsx I don't see any issue so far!

mobx is awesome.
You are awesome.

[urugator]

Just to be clear here, I would personally still prefer having ElementSize part of some Element with setRef method doing the size-ref synchronization :)

[omerman]

@brabeji Just FYI, I think you should have a look at Svelte, Its not a library for React, its a language used to build web apps, 90% simillar to JS and it has a built in mechanisem for reactive variables, I think you'd like it..:)