Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Monkeys implementation #348

Closed
Yomguithereal opened this issue Sep 19, 2015 · 28 comments
Closed

Monkeys implementation #348

Yomguithereal opened this issue Sep 19, 2015 · 28 comments

Comments

@Yomguithereal
Copy link
Owner

Here is how monkeys work:

  • Baobab.monkey creates a MonkeyDefinition instance that can be placed anywhere in the tree.
  • When the tree is written, data is parsed to find every MonkeyDefinition contained in it.
  • A "parallel" tree containing Monkey instances is created:
// For the following tree:
{
  one: {hello: 'world'},
  two: {user: 'michael', dynamic: monkey(...)}
}

// The following monkey index is created:
{
  two: {dynamic: ...}
}
  • Each monkey will then rewrite the tree's data by replacing their host node by a lazy JS getter computing the resultant data (memoized, of course).
  • If any of the deps is updated, the monkey will rewrite the lazy getter.
  • When the user write the tree, the monkeys index is updated (starting from the updated node for perf of course).
@Yomguithereal
Copy link
Owner Author

@christianalfoni, here you go.

@Yomguithereal
Copy link
Owner Author

What I'll do in the future is to enable more performant scenarios through options: like a flag in the setters informing that monkeys should be searched.

@garth
Copy link

garth commented Sep 22, 2015

It doesn't seem to be possible to chain monkeys (have one monkey read data from another) and have the entire chain update when the underlying data is changed, the second monkey in the chain doesn't seem to update.

Is this a design decision, if so I guess it should be documented?

@Yomguithereal
Copy link
Owner Author

Hello @garth, "chaining" or "recursive" monkeys should work. If they aren't then this is a bug. Can you post here your failing use case so I can assess what went wrong, please?

@garth
Copy link

garth commented Sep 22, 2015

A failing test:

  beforeEach(function () {
    this.tree = new Baobab({
      _locale: 'en',
      locale: monkey(
        ['user', 'locale'],
        ['_locale'],
        (userLocale, locale) => {
          return userLocale || locale;
        }
      ),
      userId: null,
      user: monkey(
        ['users'],
        ['userId'],
        (users, id) => users[id]
      ),
      users: {}
    });
  });

  it('locale should use _locale value when no user', function () {
    expect(this.tree.get('_locale')).to.equal('en'); // passes
    expect(this.tree.get('locale')).to.equal('en'); // passes
  });

  it('locale should use user.locale value when there is a user', function () {
    this.tree.set(['users', 1], { id: 1, locale: 'de' });
    this.tree.set('userId', 1);
    this.tree.commit();
    expect(this.tree.get(['users', 1, 'locale'])).to.equal('de'); // passes
    expect(this.tree.get(['user', 'locale'])).to.equal('de');  // passes
    expect(this.tree.get('locale')).to.equal('de');  // fails
  });

If the monkey is changed to:

      locale: monkey(
        ['users'],
        ['userId'],
        ['_locale'],
        (users, id, locale) => {
          let user = users[id];
          return user ? user.locale : locale;
        }
      ),

everything passes

@Yomguithereal
Copy link
Owner Author

Thanks @garth. I start investigating.

@garth
Copy link

garth commented Sep 22, 2015

@Yomguithereal I'm not sure if it's related, but when i try to get a value represented by a monkey, I sometimes get the monkey object instead of the value returned by the monkey.

this.tree.get('locale') // sometimes returns monkey definition instead of value returned by monkey

@christianalfoni
Copy link
Contributor

@garth and I have worked on this related to Cerebral. We figured out that it was a Cerebral debugger thing. But still, monkeys seems to have their "getters" included when not grabbing state specifically from the branch where the monkey is defined:

{
  monkeyA: monkey,
  foo: {
    monkeyB: monkey
  }
}

Grabbing tree.get() will not have getter on monkeyA, just the value. But monkeyB also has its getter. Grabbing tree.get(['foo']) monkeyB does not have its getter defined.

It does not really matter I suppose, but it is a bit inconsistent.

@Yomguithereal
Copy link
Owner Author

There clearly are some things to fix here indeed. I just need some time to see to this but I have been very busy at work lately. Sorry for the delay.

Yomguithereal added a commit that referenced this issue Sep 25, 2015
@Yomguithereal
Copy link
Owner Author

Ok @garth, I just pushed a temporary fix for your issue. Can you precise when a MonkeyDefinition is returned instead of normal data please.

To precise what the problem was: it was about order. For the time being simple cases should be solved, and if you want to avoid further problems with complex recursivity, register the monkeys in the tree in a logical order. I will need, in the future, to implement a proper dependency solving algorithm.

@christianalfoni
Copy link
Contributor

Thanks @Yomguithereal! Is this part of dev15 branch?

Btw, logical order. I did not quite understand that?

@Yomguithereal
Copy link
Owner Author

Not yet. I will release a rc3 soon when I hear from @garth about the second problem.

By logical order I mean: first register the non-recursive monkeys, then the one relying on others then the one relying on others relying on others etc.

@Yomguithereal
Copy link
Owner Author

But for the case described above, it should work fine. As a matter of fact, if you don't overcome first level recursivity, it should be fine.

@garth
Copy link

garth commented Sep 25, 2015

@Yomguithereal I'm using baobab via cerebral and after @christianalfoni made a few adjustments earlier this week it's been working very well. I haven't seen the periodic monkey issue, and I am assuming that this may have been due to an exception being thrown that caused execution to end before all monkeys had been processed?

I wrote a bunch of unit tests around my use-cases for baobab and it seems to be working as expected now.

@Yomguithereal
Copy link
Owner Author

Ok. Thanks for the feedback @garth :). Will release a new version soon. Maybe even the 2 one at last.

@Yomguithereal
Copy link
Owner Author

2.0.0-rc3 is now out.

@Yomguithereal
Copy link
Owner Author

@christianalfoni: do you see anything still rough or unstable with rc3 or can I release?

@Yomguithereal
Copy link
Owner Author

and on a side note: do you use validation (I am thinking of dropping it because it would be very easy to implement on your own outside the lib)?

@christianalfoni
Copy link
Contributor

Aha, I see :-)

I think things are looking good!

About validation I believe some developers are actively using it. I am a bit unsure where it really belongs. As you say, it would be easy to implement outside the tree.

My opinion, in regards of Cerebral, is that validation should be a service you use before setting values where validation is required. That way you can run a different set of actions/mutations if a validation fails, instead of having a general "validation error".

function myAction (input, state, output, services) {
  if (services.validate(input.email).isEmail()) {
    state.set('email', input.email);
  } else {
    state.set(['errors', 'email'], true);
    // or
    output.error();
  }
}

That said, it would really be great with some more feedback on this. Let me throw this issue into the Gitter channel and give it a couple of days?

@Yomguithereal
Copy link
Owner Author

Let's do that. Can you point me to the said gitter channel please?

@saulshanabrook
Copy link

@Yomguithereal This is the gitter channel: http://gitter.im/christianalfoni/cerebral

@Yomguithereal
Copy link
Owner Author

Thanks @saulshanabrook.

@Yomguithereal
Copy link
Owner Author

Any feeback about the implementation itself @christianalfoni?

@christianalfoni
Copy link
Contributor

you thinking about the issue you created with implementation details? Have not had time to look at it yet :-)

@Yomguithereal
Copy link
Owner Author

We actually are in said issue :).

@christianalfoni
Copy link
Contributor

haha, oh... no, not gotten the time :-) I had a read through now. It makes perfect sense to me. And I think you made the right choice of not giving the monkey a value. It does make things a lot more complex.

It would be fun to check out this self aware reducer strategy. Maybe I will make a separate small project with that. If it works as intended maybe it is something for the future, as the syntax is really sweet! :-)

Actually it does not have to be part of Baobab at all. It could work with any immutable state store. Hm hm... anyways. Great work here, looking forward to V2 :-)

@tim-steele
Copy link

Hey @Yomguithereal did you ever resolve the recursivity beyond level 1? I am having issues when a monkey is listening to a monkey who is listening to a monkey. @garth did you say that you found a solution for the recursive monkey problem beyond level 1?

@Yomguithereal
Copy link
Owner Author

The current solution is simply to register your monkeys in the dependency order and it should work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants