Typescript conversion for src/observable/map

[ibeckermayer]

The conversion of domain/session/leftpanel was being blocked by typescript's inability to pick up on the methods of BaseObservableMap when they were defined like the following in order to avoid circular dependencies:

// avoid circular dependency between these classes
// and BaseObservableMap (as they extend it)
Object.assign(BaseObservableMap.prototype, {
    sortValues(comparator) {
        return new SortedMapList(this, comparator);
    },

    mapValues(mapper, updater) {
        return new MappedMap(this, mapper, updater);
    },

    filterValues(filter) {
        return new FilteredMap(this, filter);
    },

    join(...otherMaps) {
        return new JoinedMap([this].concat(otherMaps));
    }
});

This PR fixes that problem by adding those functions as abstract functions in BaseObservableMap, and adding their default implementations in a new class called BaseObservableMapDefaults.

Each class that inherits BaseObservableMap<K, V> is then initialized with a BaseObservableMapDefaults field like:
https://github.com/ibeckermayer/hydrogen-web/blob/f0ebcfcbfe4f2a007670bafae9489df413909d68/src/observable/map/ObservableMap.ts#L25-L26

and then some boilerplate is added to call the default implementations:
https://github.com/ibeckermayer/hydrogen-web/blob/f0ebcfcbfe4f2a007670bafae9489df413909d68/src/observable/map/ObservableMap.ts#L102-L116

and some boilerplate which calls the default implementation, which is now defined in config.ts.

Update 1

Found a way to limit the boilerplate: c6efdc7

[ibeckermayer]

See import-js/eslint-plugin-import#653 (comment) (I was getting the error in the OP of that thread when trying to lint)

[ibeckermayer]

Recommended by ESlint's official guide: https://typescript-eslint.io/docs/linting/troubleshooting/#i-get-errors-from-the-no-undef-rule-about-global-variables-not-being-defined-even-though-there-are-no-typescript-errors

[ibeckermayer]

Added based on feedback in my first PR.

[ibeckermayer]

Also: I tried for the life of me to convert SortedMapList.js, but couldn't define a plausible type system that fits how its currently written.

[MidhunSureshR]

I wonder if we can use a mixin of some sort here. In index.ts where you import and export the classes, you define a function:

const NewSortedMapList = mapCreator(SortedMapList);
export { NewSortedMapList as SortedMapList };

function mapCreator(mapClass) {
    return class extends mapClass {
        sortValues(comparator) {
            return new SortedMapList(this, comparator);
        }

        mapValues(mapper, updater) {
            return new MappedMap(this, mapper, updater);
        }

        filterValues(filter) {
            return new FilteredMap(this, filter);
        }

        join(...otherMaps) {
            return new JoinedMap([this].concat(otherMaps));
        }
    };
}

Haven't tried it out but it's worth a try.

[ibeckermayer]

@MidhunSureshR I wasn't able to figure out a good way to do it using mixins, however I did find a way to eliminate a good amount of the boilerplate:

c6efdc7

[ibeckermayer]

I just noticed that there's a constrained mixin pattern that might work for us, I'll play around with that.

[ibeckermayer]

Couldn't get mixin to work

I did some messing around today, long story short is that I didn't find a good way use mixins, even when using constraints.

A core issue is that mixins don't play nicely with abstract (which our BaseObservableMap is):

type GConstructor<T = {}> = new (...args: any[]) => T;
type IsABaseObservableMap<K, V> = GConstructor<BaseObservableMap<K, V>>;

function makeTransformable<K, V, TBase extends IsABaseObservableMap<K, V>>(Base: TBase) {
    return class extends Base {
        join(...otherMaps: Array<BaseObservableMap<K, V>>): JoinedMap<K, V> {
            return new JoinedMap([this].concat(otherMaps));
        }

        mapValues(mapper: Mapper<V>, updater?: Updater<V>): MappedMap<K, V> {
            return new MappedMap(this, mapper, updater);
        }

        sortValues(comparator: Comparator<V>): SortedMapList {
            return new SortedMapList(this, comparator);
        }

        filterValues(filter: Filter<K, V>): FilteredMap<K, V> {
            return new FilteredMap(this, filter);
        }
    }
}

The returned class complains

I also ran into another typing error which I wasn't able to decipher, wherein the type system seems to get tripped up on join, complaining about the otherMaps parameter:

It's as if it doesn't realize that this and otherMaps[n] are the same type, seems to me like a bug in the type system.

What I did instead

I changed BaseObservableMapDefaults to the more descriptive BaseObservableMapTransformers, and added a more comprehensive description of why we're doing it this way.

IMO it would be nicer if we could get mixins to work in a manner similar to what I tried above, but given the issues with the type system, this is good enough. (Naturally, feel free to show me how its done if you think you see a path I missed).

I'm going to leave those merge conflicts unresolved until we figure out what's going on here.

@MidhunSureshR

[MidhunSureshR]

import { SortedMapList as _SortedMapList } from "./list/SortedMapList.js";
import { MappedMap as _MappedMap } from "./map/MappedMap";
import { FilteredMap as _FilteredMap } from "./map/FilteredMap";
import { JoinedMap as _JoinedMap } from "./map/JoinedMap";
import { BaseObservableMap as _BaseObservableMap } from "./map/BaseObservableMap.js";

// re-export "root" (of chain) collection
export { ObservableMap } from "./map/ObservableMap";
export { ObservableArray } from "./list/ObservableArray";
export { SortedArray } from "./list/SortedArray";
export { MappedList } from "./list/MappedList";
export { AsyncMappedList } from "./list/AsyncMappedList";
export { ConcatList } from "./list/ConcatList";

type GConstructor<T = {}> = abstract new (...args: any[]) => T;
type MapConstructor<K, V> = GConstructor<_BaseObservableMap<K, V>>;

function addTransformers<K, V, T extends MapConstructor<K, V>>(MyClass: T) {
    abstract class ClassWithTransformers extends MyClass {
        sortValues(comparator) {
            return new _SortedMapList(this, comparator);
        }

        mapValues(mapper, updater) {
            return new _MappedMap(this, mapper, updater);
        }

        filterValues(filter) {
            return new _FilteredMap(this, filter);
        }

        join(...otherMaps) {
            return new _JoinedMap([this].concat(otherMaps));
        }
    }
    return ClassWithTransformers;
}

export const SortedMapList = addTransformers(_SortedMapList);
export const MappedMap = addTransformers(_MappedMap);
export const FilteredMap = addTransformers(_FilteredMap);
export const JoinedMap = addTransformers(_JoinedMap);

@ibeckermayer would this work?

[ibeckermayer]

@MidhunSureshR Clever, it appears to work: 7d27d46

The primary downsides I see to this approach are:

1. It messes with the type system in a couple of ways.

One way is that from a straightforward reading of the code, it seems like we are instantiating abstract classes directly (anything that's been passed through addTransformers). Nevertheless we can get away with it, maybe because according to my editor the return type of `addTransformers is

function addTransformers<K, V, T extends MapConstructor<K, V>>(MyClass: T): ((abstract new (...args: any[]) => ClassWithTransformers) & {
    prototype: addTransformers<any, any, any>.ClassWithTransformers;
}) & T

which is something, but I'm not exactly clear what.

As an example export const ObservableMap = addTransformers(_ObservableMap); gives us a type like

const ObservableMap: ((abstract new (...args: any[]) => addTransformers<unknown, unknown, typeof _ObservableMap>.ClassWithTransformers) & {
    prototype: addTransformers<any, any, any>.ClassWithTransformers;
}) & typeof _ObservableMap

It's not exactly clear to me why the type system allows me to instantiate that, but the overarching point is that the types of these classes are now more or less inscrutable.

2. We aren't expressing that all BaseObservableMap are transformable by sortValues, mapValues, etc.

In other words, there's a semantic difference between this solution and the previous one -- previously we were saying that all BaseObservableMap have these transformers, now we're saying that only special ones which are passed through addTransformers do.

In both cases the code around this part gets a bit "weird". I'm partial to my solution from before, because outside of observable/map, all the "weirdness" goes away, in that the types just look like normal types again, and we're not playing any games with instantiating what appears to be an abstract class. I also think that "all BaseObservableMap are transformable by the following methods" is the original intent, and that is better captured by the previous solution.

[ibeckermayer]

@MidhunSureshR Any thoughts on my argument above?

[ibeckermayer]

https://matrix-org.github.io/synapse/latest/development/contributing_guide.html#sign-off

Signed-off-by: Isaiah Becker-Mayer isaiah@becker-mayer.com

[MidhunSureshR]

Sorry, it took me a bit of time to come back to this. One issue with both the approaches is that we're making non trivial changes to the code to get static typing to work. Can we instead do some thing like this? Thank you for working on this.

[ibeckermayer]

For reference, I found a bug in the MappedMap here while working on rightpanel, which I addressed here: 92ed503 (on a separate branch for now).

Sorry, it took me a bit of time to come back to this. One issue with both the approaches is that we're making non trivial changes to the code to get static typing to work. Can we instead do some thing like this? Thank you for working on this.

No worries, I will take a look in detail and see if we can get it to work. At first glance I'm skeptical, because this contains an Object.assign that looks similar to the original solution that was causing Typescript to falter (it wasn't smart enough to recognize the Object.assign would give us the assigned methods, and so wouldn't let us call them later, elsewhere). Granted I've only glanced at your proposal, so perhaps you've addressed this and I haven't understood it yet.

It appears that the general problem we're trying to solve is that we have a BaseClass for which we want to define functions which return ChildClasses that themselves extend that BaseClass, without circular imports. Perhaps there's a summary of prior art on this topic that I can find. (Just thinking out loud a bit here).

[ibeckermayer]

@MidhunSureshR

Sorry, it took me a bit of time to come back to this. One issue with both the approaches is that we're making non trivial changes to the code to get static typing to work. Can we instead do some thing like this? Thank you for working on this.

The example is close to what we currently have and causes the same problem. Namely, classes that extend BaseObservableMap don't have the Object.assigned methods according to Typescript.

[ibeckermayer]

Alright, turns out we aren't the first to encounter this general problem. I was able to solve it by arranging the export order in src/observable/map/index.ts and then always importing these classes from that file only. Based on this blog post, and have added a comment in index.ts and this warning label on all the classes.

There's a lot of commits in here now from me going down various paths, if it's not breaking a project policy then it might be best to squash and merge this one. If that's not an option let me know, and I can try and work some gitfu to clean the history up.

[MidhunSureshR]

Taking an initial look at the PR:

[ibeckermayer]

@MidhunSureshR I think this one is very close to ready.

[MidhunSureshR]

Thanks 🎉