JohnSimonsen commented 6 months ago

CustomComponentLifecycle

This lifecycle borrows a lot from the existing WrapperLifecycle.ts(credit: Mikhail Aheichyk, Nordeck IT + Consulting GmbH.) and can be implemented in the same fashion. However we felt it was needed to create a seperate lifecycle as the intended usecase is different. The idea behind the CustomComponentLifecycle is that it should "swap" an existing standard element/matrix-react-sdk component with a custom component in it's place by writing a module. This deviates from the WrapperLifeCycle's usecase to wrap a component and append custom components as children to the wrapper / sibling of the wrapped component.

Changes

Added CustomComponentLifecycle.ts
Added CustomComponentLifecycle.test.tsx
Updated types.ts - Added CustomComponentLifecycle to AnyLifecycle
Updated README.md - Added description of CustomComponentLifecycle

Why is it needed?

Code readability - A component wrapped in </customComponentOpts.CustomComponent>, indicates that you have a lifecycle hook to access what happens with this component.
One can easily deviate from the upstream with low-cost maintenance
Opens a new door for people who wish to customise components
- It is a "catch-all" case for customization, giving the developers access to customise any wrapped component using modules instead of forking the repository.
- opens up for alternative open sourced components, which can be installed using build-config.

Implementation example

As an experiment / Proof of concept i attempted the following: Can I swap the UserMenu component in matrix-react-sdk seamlessly, with a custom module with custom elements and behaviours/functionality. Still retaining the original state, children and reactivity?

Yes! ✔

The highlighted elements are (customisations) proof of a "modified" UserMenu. The reactivity from child to expand panel etc, are retained. And since I based my customisation on the existing standard UserMenu all the functionality was transfered (i.e light/dark theme, default menu options etc.)

1. The module custom-usermenu-module

I created a new Module called CustomUserMenuModule with the following dependencies

Adding a dependency to matrix-react-sdk and matrix-js-sdk allowed me to harness the full type library and stores from those packages. Meaning I could copy the entire UserMenu.ts file from matrix-react-sdk to my modulecustom-usermenu-module without any errors. I could then make my customisations directly in my module.
By having a dependency to the matrix-react-sdk you can create just as powerfull components as in a fork. Allowing you to re-use sub-components like buttons etc.
The element-web dependency to the module is of course just an optional "build-config" dependency.

Implementation of the module:

export default class CustomUserMenuModule extends RuntimeModule {
    private readonly CustomComponent: CustomComponentOpts['CustomComponent'] = React.Fragment;
    public constructor(moduleApi: ModuleApi) {

        super(moduleApi);

        this.CustomComponent = function CustomComponent({children}){

            // Get the component the wrapper is containing / i.e get the component to be swapped.
            let usermenu: any = React.Children.toArray(children)[0]

            return(
             <CustomUserMenu isPanelCollapsed={usermenu.props.isPanelCollapsed}>
                {usermenu.props.children}
             </CustomUserMenu>
            )
        }

        this.on(CustomComponentLifecycle.UserMenu, this.onCustomComponent);
    }
    protected onCustomComponent: CustomComponentListener = (CustomComponentOpts) => {
        CustomComponentOpts.CustomComponent = this.CustomComponent;
    }
}

Note: Notice that the implementation uses the lifecycle proposed in this PR.

2. The Implementation example of the lifecycle in matrix-react-sdk (UserMenu)

📄matrix-react-sdk>src>components>views>SpacePanel.tsx 👇 (Lines with a "👈" are relevant in this implementation)

(...)
const SpacePanel: React.FC = () => {
    const [dragging, setDragging] = useState(false);
    const [isPanelCollapsed, setPanelCollapsed] = useState(true);
    const ref = useRef<HTMLDivElement>(null);
    useLayoutEffect(() => {
        if (ref.current) UIStore.instance.trackElementDimensions("SpacePanel", ref.current);
        return () => UIStore.instance.stopTrackingElementDimensions("SpacePanel");
    }, []);

    useDispatcher(defaultDispatcher, (payload: ActionPayload) => {
        if (payload.action === Action.ToggleSpacePanel) {
            setPanelCollapsed(!isPanelCollapsed);
        }
    });

    const isThreadsActivityCentreEnabled = useSettingValue<boolean>("threadsActivityCentre");

    const customUserMenuOpts: CustomComponentOpts = { CustomComponent: React.Fragment }; 👈-  Create wrapper
    ModuleRunner.instance.invoke(CustomComponentLifecycle.UserMenu, customUserMenuOpts) 👈 - Invoke ModuleRunner
    return (
        <RovingTabIndexProvider handleHomeEnd handleUpDown={!dragging}>
            {({ onKeyDownHandler, onDragEndHandler }) => (
                <DragDropContext
                    onDragStart={() => {
                        setDragging(true);
                    }}
                    onDragEnd={(result) => {
                        setDragging(false);
                        if (!result.destination) return; // dropped outside the list
                        SpaceStore.instance.moveRootSpace(result.source.index, result.destination.index);
                        onDragEndHandler();
                    }}
                >
                    <nav
                        className={classNames("mx_SpacePanel", { collapsed: isPanelCollapsed })}
                        onKeyDown={onKeyDownHandler}
                        ref={ref}
                        aria-label={_t("common|spaces")}
                    >
                        <customUserMenuOpts.CustomComponent> 👈 Wrap the component to be "override-able"
                            <UserMenu isPanelCollapsed={isPanelCollapsed}>
                                <AccessibleTooltipButton
                                    className={classNames("mx_SpacePanel_toggleCollapse", { expanded: !isPanelCollapsed })}
                                    onClick={() => setPanelCollapsed(!isPanelCollapsed)}
                                    title={isPanelCollapsed ? _t("action|expand") : _t("action|collapse")}
                                    tooltip={
                                        <div>
                                            <div className="mx_Tooltip_title">
                                                {isPanelCollapsed ? _t("action|expand") : _t("action|collapse")}
                                            </div>
                                            <div className="mx_Tooltip_sub">
                                                {IS_MAC
                                                    ? "⌘ + ⇧ + D"
                                                    : _t(ALTERNATE_KEY_NAME[Key.CONTROL]) +
                                                      " + " +
                                                      _t(ALTERNATE_KEY_NAME[Key.SHIFT]) +
                                                      " + D"}
                                            </div>
                                        </div>
                                    }
                                />
                            </UserMenu>
                        </customUserMenuOpts.CustomComponent> 👈 Close customUserMenu wrapper
                        <Droppable droppableId="top-level-spaces">
                            {(provided, snapshot) => (
                                <InnerSpacePanel
                                    {...provided.droppableProps}
                                    isPanelCollapsed={isPanelCollapsed}
                                    setPanelCollapsed={setPanelCollapsed}
                                    isDraggingOver={snapshot.isDraggingOver}
                                    innerRef={provided.innerRef}
                                >
                                    {provided.placeholder}
                                </InnerSpacePanel>
                            )}
                        </Droppable>

                        {isThreadsActivityCentreEnabled && (
                            <ThreadsActivityCentre displayButtonLabel={!isPanelCollapsed} />
                        )}
                        <QuickSettingsButton isPanelCollapsed={isPanelCollapsed} />
                    </nav>
                </DragDropContext>
            )}
        </RovingTabIndexProvider>
    );
};
(...)

Note: Notice that the implementation uses the lifecycle proposed in this PR.

Signed-off-by: John Tore Simonsen jts@verji.com

JohnSimonsen commented 6 months ago

@MidhunSureshR @t3chguy I am having trouble requesting a code reviewer. Could any of you help me out?

JohnSimonsen commented 5 months ago

@robintown @t3chguy Could I have another go at the workflow? I think the lint-errors should be fixed now.

JohnSimonsen commented 5 months ago

I personally don't have a lot of context for how this project works, and to what extent it already embraces things that feel a little hacky, but that's my impression of this solution.

Am I right that this only allows customization of class components? While many important components in matrix-react-sdk are classes, nowadays we're trying to only write functional components, so I'd much prefer a solution that generalizes to them as well. If it's not possible to customize functional components in a similar manner, then adding this feature could create a counter-productive incentive to rewrite our functional components as classes.

I'll need to defer final judgement to someone like @t3chguy who may have more experience with the module API, but hopefully my thoughts are of some use.

In theory it shouldn't matter if it is a class or a functional component. As long as you are able to put the customComponentOpts wrapper around it. I haven't personally tried on a functional component, just yet. But i'll be happy to provide an example if necessary. I completely agree that handling customisations differently for different component types would be an anti-pattern.

I would greatly appreciate if you could also have a look at the example provided in the PR-description on how it could be implemented in matrix-react-sdk as well. If you / others in the Matrix team don't like the way the implementation looks - It would be good to catch it now, so we can adjust the lifecycle ahead of time.

As for some more context on the proposed solution: I did some alternative experimentation on how to achieve the end goal of component override/customisation(Factory implementations etc.). But in my opinion I believe this was the best solution I found, as it has notable upsides:

Low impact hook implementation in matrix-react-sdk
Follows same structure as the WrapperLifecycle - which has already been accepted
Would solve most customisation needs
Ease of use / Versatile

That being said, I'd be willing to put down the work if there are any suggestions on how to improve it.

t3chguy commented 5 months ago

I'll need to defer final judgement to someone like @t3chguy who may have more experience with the module API, but hopefully my thoughts are of some use.

I have zero experience with the module API and very little context aside that so please do not block this one me.

JohnSimonsen commented 5 months ago

@robintown - I extended the tests to include usecase of how you can just as easily wrap and swap a functional component with the same method. I also took your comment into account and got rid off the casting in the constructor of module, and I dropped the "any", that was pointed out in one of the tests (Although was solved by casting the result. As React.Children.toArray() has multiple possible return values.)

If neither of you has experience with the module-api. Maybe I could suggest adding @germain-gg as a code reviewer? By the looks of it he handled and accepted this PR: https://github.com/matrix-org/matrix-react-sdk-module-api/pull/13 This PR builds on the same ideas.

The mechanics of how this works, can be confusing. And I realize that the PR-description is quite extensive; I'll try to give a short and concise explanation, to ease the burden of mental gymnastics, when familiarizing with the concept. (TL;DR)

1. Initialization of CustomComponentOpts: const customComponentOpts: CustomComponentOpts = { CustomComponent: React.Fragment };

We initialize customComponentOpts with React.Fragment, indicating an initial state where no modification is applied to the rendered output.

2. Invocation of ModuleRunner: ModuleRunner.instance.invoke(CustomComponentLifecycle.UserMenu, customComponentOpts)

The ModuleRunner is invoked with a lifecycle hook and customComponentOpts.
If no module has registered a handler for the lifecycle. The wrapper remains an empty React.Fragment - containing original markup/component.
However we can implement a handler in a module to replace the React.Fragment with a function.
- In this function we can extract state from it's children.
- And we gain the ability to update the contents of the wrapper itself - for example by returning a CustomComponent instead

This feature could help resolve reported issues, like: https://github.com/matrix-org/matrix-react-sdk-module-api/issues/21

langleyd commented 5 months ago

I marked this as needs product/design as I think we would need their alignment/approval to understand if this is a way in which we want extensibility to work.

JohnSimonsen commented 5 months ago

@langleyd Understandable, thank you for getting back to me. Please let me know if I can be of assistance in any way.

Our team (Verji) have chosen to move ahead with this feature on our fork, as it provides us with a "cleaner", organised structure of custom code vs upstream, and drastically lessens the burden of updating/maintaining our fork.

langleyd commented 4 months ago

Hi @JohnSimonsen , This is a very generic solution and its somewhat unclear what the implications would be on our apps overall maintainability if we were to take this approach. It’s great that it helps you manage your fork but our product’s module apis(including on other platforms) are still experimental and without a more evolved strategy/guidance we can't accept this at this point in time.

JohnSimonsen commented 4 months ago

@langleyd Thanks for the update, a bit disappointed with the decision. It would've been a huge win to get this into the upstream, for us at least. And probably others, as it would allow the community to be able to create detailed customisations on component level, completely without having to fork the project.

We'll stay tuned if there are better solutions presented at a later time. In the meantime we'll get some mileage on this solution, and if you're interested - we'll be happy to give you an update later with more mature results.

Appreciate you took the time to look into it, @langleyd! 😊

matrix-org / matrix-react-sdk-module-api