amcdnl/ngrx-actions

Perf Issue

amcdnl opened this issue · 5 comments

The following code creates a new select on EVERY get. This is performance issue.

     Object.defineProperty(target, name, {
        get:() => store.select(fn),
        enumerable: true,
        configurable: true
      });

I tried to only create it once like:

      Object.defineProperty(target, name, {
        get: function() {
          if (!select) {
            select = createSelect();
          }
          return select;
        },
        enumerable: true,
        configurable: true
      });

however in real world unit tests, it started failing. I'm not sure the EXACT cause of this but I'm thinking its because the select is created statically and if u are recreating a class over and over again the select is not pointing to the new instance.

If that is true, I dont know a good way to do this because we need a hook to know when the class was created again and while we could do ngOnInit hook, it would get shaken out in AoT if the user didn't define this.

@albyrock87 - any thoughts?

OMG you're right!!
I think that this is the only way to do it:

function Select(fn) {
  return function(target: any, name: string): void {
    const createSelect = function () {
      // "this" is the final object reference
      console.log('new creation - must be logged once', this);
      return store.select(fn);
    };

    const selectorFnName = '__' + name + '__selector';

    const getter = function () {
      // "this" is the final object reference
      return this[selectorFnName] || (this[selectorFnName] = createSelect.apply(this)); 
    };

    // Redefine property
    if (target[selectorFnName]) {
      throw new Error('You cannot use @Select decorator and a ' + selectorFnName + ' property.');
    }
    if (delete target[name]) {
      console.log('define property');
      Object.defineProperty(target, selectorFnName, {
        writable: true,
        enumerable: false,
        configurable: true
      });
      Object.defineProperty(target, name, {
        get: getter,
        enumerable: true,
        configurable: true
      });
    }
  };
}

class Foo {
  @Select()
  bar;
}

var f = new Foo();
f.bar;
f.bar;
f.bar;

CLEVER! I'll make this change and see if it fixes the issues in prod env.

Uh ok... I was writing a pull reqest. No problem.. :)

Published and verified in prod it works! THANKSSSSS!

what's ur email, i'd like to add u to my slack channel.