Selector and Action Property Decorators Proposal

[jmzagorski]

I'm submitting a feature request

• Browser:
  all

• Language:
  all

Current behavior:
There are no property decorators

Expected/desired behavior:

• What is the expected behavior?

Some ideas for these selectors include, but not limited to:

The select decorator
Should:
🔲 Subscribe to the store when the view model is added
🔲 Select the slice of state
🔲 Call the change handler method after the value is set
🔲 Unsubscribe to the store when the view model is removed
Could:
🔲 Pass additional props
🔲 Pass the view model as additional props
🔲 Pass an observable
🔲 Pass a POJO
🔲 Compare the two values with the default === before running the change handling

The action decorator
Should:
🔲 Register the action when the view model is added
🔲 Unregister the action when the view model is removed

• What is the motivation / use case for changing the behavior?
  With connectTo you have to create a property anyway with typescript or you will get a compile time error. There is no way to register actions with the store automatically in the background, similar to mapToDispatch with redux. I am sure there are others i am missing...

This should also satisify #69 without introducing a breaking change to the connectTo decorator.

[jmzagorski]

I'll start off with a simple approach, very similar to connectTo. Below are two rough draft proposals that decorate aurelia-router activate and deactivate. This is just an example, a better solution would be to bind the selector and action after the constructor runs

The select decorator
Should:
❌ Subscribe to the store when the view model is added. This example uses activate
✔️Select the slice of state
✔️ Call the change handler method after the value is set
✔️Unsubscribe to the store when the view model is removed
Could:
✔️Pass additional props?
❌ Pass the view model
❌ Pass an observable: personally i do not like being dependent on observables and observable operators in my selectors
✔️Pass a POJO instead?
✔️Compare the two values with the default === before running the change handling?

import {
  Store
} from 'aurelia-store';
import {
  Container
} from 'aurelia-framework';

// TODO this uses activate & deactivate, but really we should
// somehow bind after the ctor runs
export function select<T, R = T | any>(
  selector?: (state: T, ...selectorArgs: any[]) => R,
  ...selectorArgs: any[]
  ): PropertyDecorator {
  return (target: any, key: string): void => {
    let subscription = null;
    const store = Container.instance.get(Store) as Store<T>;
    const originalSetup = target.activate
    const originalTeardown = target.deactivate;

    target.activate = function () {
      subscription = store.state.subscribe((state: T) => {
        const changeHandler = key + 'Changed';
        const currentSlice = this[key];
        const newSlice = this[key] = selector(state, ...selectorArgs);

        if (newSlice !== currentSlice && changeHandler in this) {
          this[changeHandler](this[key], currentSlice);
        }
      });

      if (originalSetup) {
        return originalSetup.apply(this, arguments);
      }
    }

    target.deactivate = function () {
      subscription.unsubscribe();

      if (originalTeardown) {
        return originalTeardown.apply(this, arguments);
      }
    }
  };
}

The action decorator
Should:
❌ Register the action when the view model is added. This example uses activate
✔️ Unregister the action when the view model is removed

import {
  Container
} from 'aurelia-framework';
import {
  Reducer,
  Store,
  dispatchify
} from 'aurelia-store';

// TODO this uses activate & deactivate, but really we should
// somehow bind after the ctor runs
export function action<T>(action: Reducer<T>): PropertyDecorator {
  const store = Container.instance.get(Store) as Store<T>;

  return function (target: any, key: string) {
    const originalSetup = target.activate;
    const originalTeardown = target.deactivate;

    target.activate = function () {
      store.registerAction(action.name, action);
      this[key] = dispatchify(action);

      if (originalSetup) {
        return originalSetup.apply(this, arguments);
      }
    }

    target.deactivate = function () {
      store.unregisterAction(action);

      if (originalTeardown) {
        return originalTeardown.apply(this, arguments);
      }
    }
  }
}

Usage:
home/home-grid (View Model)

import {
  ClearSelectionAction,
  clearSelection,
  getData,
  ITEM
} from '../home';
import {
  Item
} from '../models';
import {
  select,
  action
} from 'aurelia-store'; // PROPOSAL ONLY, NOT ACTUALLY IN LIB

export class HomeGrid {
  @select(getData, ITEM.GRID)
  items: Item[];

  @action(clearSelection)
  clearSelection: ClearSelectionAction;
}

home/index

export * from './actions';

export const ITEM = {
  GRID: 'items'
};

home/actions

import {
  State
} from './state'
import produce from 'immer';

// probably a better way: used to make sure the arguments in the view model are correct
export type ClearSelectionAction = (
  grid: string) => void;

// action
export function clearSelection(state: State, grid: string, index: number): State {
  return produce(state, draft => {
    draft.grids[grid].selections = [];
  });
}

// selector: could keep in same file as actions or separate out to own file, but either way the selector and
// action both have to know about the shape of the state
export const getData =
  <T>(state: State, grid: string): T[] =>  state.grids[grid] && state.grids[grid].data;

[zewa666]

Can you share the code sample of ../actionsAndSelectors so that we get an idea of what you're referencing down there in your decorators. Additionally ITEM.GRID is likely just a const name or such right? So an additional parameter passed to your getData selector?

[bigopon]

It seems to me we are relying in Container.instance, which is not bad, but won't work in multiple Aurelia instance scenario. I think we should target created life cycle of custom element / custom attribute, that promises to be better for those scenario?

Edit: I'm not sure if it will work out, but we can have access to the CE / CA instance container, and also able to get to the root container:

function getRootContainer(container) {
  while (container.parent) {
    container = container.parent;
  }
  return container;
}

[jmzagorski]

@zewa666 I updated my example to be more complete while trying to keep it simple (e.g. I removed the double underscore, which i use for namespacing actions, similar to the / in redux). I pasted that prior example in haste and tried to make it dead simple, but that can be more confusing than a complete example

@bigopon I always thought it wrong to use the container in a decorator, but could never find a way around it. If there is a better way we should definitely use it. The problem with using the container inside of a life cycle method to set everything up is aurelia-router's activate method, which fires before any of the lifecycle methods. In #69 @Vheissu spoke of setting his subscriptions up in the constructor because bind was too late. From that, I thought it best to set everything up as quickly as possible to avoid reference issues and so it is immediately available to the user. I could have went the route of asking the user which method they prefer to set everything up, but I wanted to keep a simple configuration, at least for now.

Since the current setup would cause issues with multiple instances, would getRootContainer work outside of a life cycle method? What are some other alternatives?

Update: For the @select I thought about using a getter in Object.defineProperty, but I did not have time to think about the best way to deal with aurelia's dirty checking (e.g. could we use declarePropertyDependencies somehow?). That would mean we can move the container into the getter and still setup a subscription in created to fire the change handler method.

[jmzagorski]

@zewa666 have you thought about/tried adding a static instance property to the store so we do not have to depend on the container in decorators? Seems it is in dispatchify as well. Would that solve the multiple Aurelia app issue?

[zewa666]

Hi @jmzagorski

first of all I'm sorry it took me so long to reply to your previous comment.
Looking at what happens I feel like this might turn into a larger refactor-intense task. Besides that the current store state would suffer of too many options to do the same.
So instead of trying to get this right away into the store, how about we instead start first with a companion plugin for the Aurelia Store plugin. This would not only allow easier development and re-iteration on a custom repo but also help us to get the integration story of partner plugins covered.
How do you think about that?

As for the recent comment, I have absolutely no idea what you're talking about :) A static instance property of what, the store or the container?
Also I was unaware of the fact that the way we obtain the container could interfere with a multi-app context. Maybe this is better suited in an issue for itself?

[jmzagorski]

No worries. I know how busy life can be and i do not think this feature is super urgent. I am all for companion plugins as that is a good place to experiment and get feedback without changing the store. After all that is what most of us do in our own private app anyway when looking for new features.

As for the instance, it would look something like this:

export class Store<T> {
  static instance: Store<T>;

  constructor(private initialState: T, options?: Partial<StoreOptions>) {
     Store.instance = this;
  }
}

and then in the decorator or dispatchify

export function dispatchify<T, P extends any[]>(action: Reducer<T, P> | string) {
  const store = Store.instance;

  return function (...params: P) {
    return store.dispatch(action, ...params) as Promise<void>;
  }
}

[catsalive]

I think this is a really great proposal. I think the highlight for me would to be able to pass props or the view model to the selector. Mainly, I'm thinking of bindables.

[bigopon]

@jmzagorski thanks for your input 👍 . Recently we have had some commits to drop the usage of Container.instance so it should be good by now.

[jmzagorski]

@bigopon thanks this looks great and i've implemented the logic in my own decorators. The only issue I have is integrating the decorators with aurelia-router since activate gets called before created so no actions or selectors are available yet. You can easily handle this problem when the view model is created by putting action calls in created or attached. However, if your view models have an activationStrategy of invokeLifecycle, then you need your actions called in activate for any subsequent calls to activate, or create some funky logic to check if the actions exists yet.

Seems like this is a stale issue anyway and hopefully there will be some more thought into this later if decorators are considered.

[bigopon]

That's interesting. activate is a tough one. container is theoretically ready, but it's not reachable easily.
@EisenbergEffect I think this is a good example of exposing container on html behavior instances.

[EisenbergEffect]

@bigopon Correct me if I'm wrong, but we've addresses this already for vNext via access to the $controller property, right? For vCurrent, should we try to reverse link the controller back to the component as well? I can't recall if the container is reachable from there, but that might be one way to do it. Another way would be to have the requirement that components request the container be injected and then have that as a convention for the decorator.

[bigopon]

Yes, we may just need to link to container only, or we can link to he controller for uniformity with vnext. For vCurrent, there are two viable ways to reach container/controller: injecting container or hooking into created and access it from the view. Both former require dev to explicitly add a certain code to enable it, and the later does not work with custom attribute.
cc @fkleuver

[bigopon]

Also, j think during activate, only view model has been created

[fkleuver]

In vNext controllers and view models are linked in both directions, and the container sits on the controller. That still needs a dehydrate lifecycle to prevent memory leaks and I'm not sure if we've got a suitable place in vCurrent for a cleanup like that. May want to be careful with mimicking vNext too much there.

In vCurrent the viewModel is placed on the container, and the container is placed on child routers. You can get the child router via the lifecycleArgs, so via .router.container.viewModel that should give you all necessary pieces right?

[bigopon]

It's not only for routing, but also for various use cases. And I think, even, router is not one of them as you rarely need to have access to container while already have access/dealing to router.

The access to container is to ensure many dependencies are resolvable from each instance of html resources (custom element, custom attribute) to support scenario like this. We can treat it like a semi private API and warn about their usage.

About memory leak, I don't think it will be the case, when view model is gone, it should be GC'able. Normally view model has reference to pretty much everything with observer in __observers__ any way.