More simple lightbox setup for SDK and compatible with custom History

[MadLittleMods]

More simple lightbox setup for SDK and compatible with custom History

Changes to go along with matrix-org/matrix-viewer#12

Split out from #653

Todo

• Test this out in the real Hydrogen to make sure everything still works

[MadLittleMods]

Replacing all of the previous boilerplate to setup the lightbox with this composable function that external consumers can also use in the SDK.

This makes setting up the lightbox in your project that uses the SDK as simple as a setupLightboxNavigation(...) function call. See the usage in matrix-org/matrix-viewer#12

[MadLittleMods]

Since we're using a custom History implementation in matrix-org/matrix-viewer#12 to get hashes relative to the room, we need to use that custom History over the default one from platform.

The only other place we use platform.history is in src/platform/web/main.js#L38

[bwindels]

I think it would be better to inject your custom history through platform.history.

[MadLittleMods]

See #749 (comment) for why we're doing this now.

[MadLittleMods]

Added the option of passing in eventEntry directly since we have access to all of the events already in matrix-public-archive and we don't need or want to load more events.

Previously, this only allowed passing eventId which loaded in the full event.

[MadLittleMods]

This is to allow this function to run when server-side rendered in linkedom. It's ok for this function to return undefined. See shared/lib/archive-history.js for how this is integrated.

If this isn't desired, I can always wrap super.get() in my custom History instance and default that way.

[bwindels]

I think it'd be better if you add your custom logic in your custom instance. It's going to be hard to not have regressions for some arcane DOM impl we never test for.

[bwindels]

I think it would be better to inject your custom history through platform.history.

[bwindels]

I think it'd be better if you add your custom logic in your custom instance. It's going to be hard to not have regressions for some arcane DOM impl we never test for.

[bwindels]

Hmmm, I think this kind of dynamic mixin programming makes the code as a whole a lot less readable. Also, this will prevent us to make the track and disposeTracked method protected once we're fully converted to typescript.

I can see a case for having a generic helper method on ViewModel to setup a child view model based on a navigation observable, but I'd do it differently, something like this:

class ViewModel {
	bindChildToNavigationSegment(segmentName, setter, getter, vmMapper, publicPropName) {
		const update = value => {
			if (getter()) {
	            setter(this.disposeTracked(getter()));
	        }
	        if (value) {
	            setter(this.track(vmMapper(value)));
	        }
	        this.emitChange(publicPropName);
		}

		const observable = this.navigation.observe(segmentName);
        this.track(observable.subscribe(update));
        update(observable.get());
	}
}

[MadLittleMods]

To make it make sense it my head, here would be the usage for lightbox (adapted to using object parameters since there is so many flying around here):

this.bindChildToNavigationSegment({
  segmentName: 'lightbox',
  setter: (newValue) => {
    this._lightboxViewModel = newValue;
  },
  getter: () = {
    return this._lightboxViewModel;
  },
  vmMapper: (eventId) = {
    return new LightboxViewModel(this.childOptions({eventId, room}));
  },
  publicPropName: 'lightboxViewModel'
});

bindChildToNavigationSegment does make a lot of sense to capture this abstraction because it's so common 👍

The usage seems like too much boilerplate for what a SDK consumer cares about though.

---

I think we could boil it down to:

this.bindChildToNavigationSegment({
  segmentName: 'lightbox',
  vmMapper: (eventId) = {
    return new LightboxViewModel(this.childOptions({eventId, room}));
  },
  publicPropName: 'lightboxViewModel'
});

class ViewModel {
  bindChildToNavigationSegment({ segmentName, vmMapper, publicPropName }) {
        // Store the `ViewModel` under a symbol so no one else can tamper with
        // it. This acts like a private field on the class since no one else has the
        // symbol to look it up.
      let viewModelSymbol = Symbol(publicPropName);

      // On the given `vm`, create a getter at `publicPropName` that the
        // `ViewModel` is exposed at for usage in the view.
        Object.defineProperty(vm, publicPropName, {
            get: function() {
                return vm[viewModelSymbol];
            }
        });
        
    const update = value => {
      if (vm[lightboxViewModelSymbol]) {
              vm[lightboxViewModelSymbol] = this.disposeTracked(getter());
          }
          if (value) {
              vm[lightboxViewModelSymbol] = this.track(vmMapper(value));
          }
          this.emitChange(publicPropName);
    }

    const observable = this.navigation.observe(segmentName);
        this.track(observable.subscribe(update));
        update(observable.get());
  }
}

If some use case needs getter/setter we could have those as extra options and store it for them by default as above ^

[MadLittleMods]

@bwindels Does the boiled down version look reasonable?