[v2] Facet API proposition

[jacomyal]

I have been discussing the new facets API with @Yomguithereal, and here are the problems with the current propositions:

• I think it is too dangerous to prefix keys in the tree as does the current implementation, since I do not always know all the keys I am manipulating in my tree, and I cannot guarantee that there are no key starting with a $ (mostly when it comes from JSON documents).
• @christianalfoni's solution fixes this issue, but having a function to return an object - and 99.9% of the time will do absolutely nothing else, seems a bit weird to me.

Another idea would be to use a facets factory. This would give us something like:

var Baobab = require('baobab');

var tree = Baobab({
  user: {
    name: 'John',
    surname: 'Smith',

    // Facet:
    fullname: Baobab.facet({
      cursors: {
        name: ['user', 'name'],
        surname: ['user', 'surname']
      },
      get: function(data) {
        return data.name + ' ' + data.surname;
      }
    }
  })
});

What do you think about this?

[Yomguithereal]

Related to #240.

[Yomguithereal]

@ivan-kleshnin @saulshanabrook @AutoSponge any opinion about this?

[Yomguithereal]

I guess that one of the perks of this method would also be to be able to create out-of-the-tree facets also, if needed.

[ivan-kleshnin]

I cannot guarantee that there are no key starting with a $ (mostly when it comes from JSON documents)

Yes, this may be a problem in some occasional cases.

since I do not always know all the keys I am manipulating in my tree

How this matters? If key with dollar is stored in variable it still works.
I don't understand.

Another idea would be to use a facets factory

Here factory just returns data, so we should either discuss it's specific structure (back to the root question of the subject) or specific instance (back to OOP).

[Yomguithereal]

since I do not always know all the keys I am manipulating in my tree

Baobab will, by convention, understand that a $ prefixed key is a holding computed data. This was made so to avoid weird heuristics on the given data.

[ivan-kleshnin]

Baobab will, by convention, understand that a $ prefixed key is a holding computed data. This was made so to avoid weird heuristics on the given data.

I remember that.

I've read

I think it is too dangerous to prefix keys in the tree as does the current implementation, since I do not always know all the keys I am manipulating in my tree, and I cannot guarantee that there are no key starting with a $ (mostly when it comes from JSON documents).

as containing two reasons, not one.

[Yomguithereal]

@christianalfoni any insight on this after your experience with immutable-store?

[AutoSponge]

I'll show you something (albeit highly edited) that I used in a recent app and why we liked it:

//from the file appModel.js
import Baobab from 'baobab';
    //facets using constants are based on simple cursors with an array path,
import * as cursor from 'app/constants/path-constants';
    //facets with string names are based on other facets
import * as facet from 'app/constants/name-constants';

export default new Baobab({}, {
  autoCommit: true,
  asynchronous: false,
  synchwrite: true,
  facets: {

    // SIMPLE Facets
    currentEntryIndex$: {
      cursors: {
        data: cursor.CURRENT_ENTRY_INDEX$
      },
      get: utils.getData
    },
    entries$: {
      cursors: {
        data: cursor.ENTRIES$
      },
      get: utils.getData
    },
    user$: {
      cursors: {
        data: cursor.USER$
      },
      get: utils.getData
    },

    //COMPLEX FACETS
    entry$: {
      facets: {
        currentEntryIndex: facet.CURRENT_ENTRY_INDEX$,
        entries: facet.ENTRIES$
      },
      get: ({entries, currentEntryIndex}) => pluck(currentEntryIndex, entries)
    }
});

This code was very easy to follow, extend, and create conventions around. It supplied all of the data for the app's components. Most components don't care if they're using a facet or a cursor as long as they get updated when state changed. The only time it matters is when a component is responsible for updating the tree, in which case they need a cursor.

Using the same constants file keeps cursor paths easily manageable. Each component was introduced to appModel (the tree) during bootstrap. Upon instantiation, views would gain access to the data they needed using a line like this:

const {entry$, user$} = appModel.facets;

Then all updates were passed from the appModel to the view model:

//this helper function just uses the identity fn as a transformer
updateFromCursor('user', user$);

Conventions:

If someone had a problem with their component, it was easy to see the view's state and track down the flow that created that state. From above, 'user' is the view model's reference to user data. 'user$' is the app model's emitter pushing updates.

Conventions help when things get complicated:

In this (seemingly) simple game, one facet merges 8 other facets and the view model that uses it has another 10 facets it needs to render correctly. It then holds another dozen or so models that only pertain to the view (it's not persisted back to the app model). There's only a single cursor in that view. Therefore, it can only update 1 thing in the app model. You can find what it has access to update and where it happens in seconds--despite the complexity of the view's logic and many handlers.

@jacomyal 's code looks very similar to what we did for our game (which is now live in prod). Our only difference is that we wanted a clear separation of app model and view model references so we added $ to indicate it was an emitter. But both cursors and facets use $. What determines access is how you generate the reference--either by grabbing a facet from the app model or selecting a cursor from the app model.

Again, I'm of the opinion that most everything works as intended. I wouldn't want to have key names forced upon me.

[christianalfoni]

Just putting in my thoughts :-)

The reason a function was chosen for immutable-store was to ensure it being handled correctly, as it is a different type. I agree that $ convention can cause problems. That said, I also see the argument of a function just returning the same type of object seems overkill :-) But its better I think.

It also opens up for doing other things, like passing in values to that function, if needed for some reason.

Just an imaginative example:

{
  myMapping: function (mappings) {
    return mappings.once({
      ...
    });
  }
}

It also makes you able to define the "facet" without a dependency to the library itself, just use plain javascript. Not that important, just less dependencies.

[Yomguithereal]

Hello @christianalfoni. I am currently converting v2 to use an equivalent of @jacomyal's proposition (instead of facets we'll have monkeys or dynamicNodes for the less fancy ones). I get your point about using functions instead of a library utility but I guess you can create your dynamic nodes as you wish and then feed them to the constructor without further ado anyway. I like the idea of passing something to the function however. This could prove useful in the future. Could make a solution for the relative paths some people want to see in the library. Anyway, the idea here is mostly to drop this sigil-like $ nonsense and avoid some unwanted key collisions.

[saulshanabrook]

I think passing values to facets would be very useful, like @christianalfoni proposed.

Sure you can create facet's but that seems more cumbersome and verbose than allowing input directly to the facets.

[Yomguithereal]

Could you precise what you mean @saulshanabrook? What could we pass to the facets exactly?

[Yomguithereal]

Also @christianalfoni, the idea to free ourselves from the $ sigil etc. was not stranger to let the possibility to have facets sit outside of the tree for convenience.

[saulshanabrook]

Like for example, I have a lot of newSystem objects in my tree, in different paths. I want to check if each is valid. When I only had one, I made a $newSystemValid facet. But not that there are many, I moved that to a plain javascript function and don't have it as a facet anymore. If I could have a $newSystemValid facet that took input somehow, then I could move this back to the tree.

In the current syntax, maybe this could be supplied by passing an object after the facet, when getting it. Like tree.get('path', 'to', '$facet', {input: 1}). Not sure if that would make sense in the new syntax.

[Yomguithereal]

Ok, I see. What you'd want is a way to pass arbitrary data to a facet somehow. However the last line you wrote would not work in v2's current installment because it means you want to get something beneath the return value of your facet. Something along the way of "I need to get the first element matching {input: 1} in the array provided by $facet".

I guess this is basic function-land here since this would mean that the return value of a node of the tree would be indeterminate and this is not possible. We would then have something like a "quantic" node whose value could be a lot of different things. I guess facets etc. shouldn't be something else than a reduction of the current state.

[Yomguithereal]

Any opinion about this @christianalfoni @jacomyal?

[Yomguithereal]

@saulshanabrook, I guess you could solve your problem by creating a facet returning a kind of index of valid items in your collection then using a selection pointing beneath the facet. Something like:
tree.get('path', 'to', '$facet', itemId);.

[saulshanabrook]

@Yomguithereal Yeah that would work.

[christianalfoni]

Haha, I am loosing my mental image here :-)

Yes, I agree that facets are just a reduction of current state... you know... what would be interesting is to remove dependencies. Make it self aware.

{
  myStateValue: function (value = [], get) {
    return value.map((id) => get('projects', projects[id])); 
  }
}

Okay, hang in there, just exploring :-)

1. Initially all facets will be run when tree is created, which actually sets their initial value (as seen above). A set to the facet will change its value
2. If you bring in state from localStorage/server that would be hydrated, using like tree.hydrate(newState), which is needed to even update a tree with facets defined in it (merge would override)
3. The get argument is what makes the facet self aware. As the facet runs all paths grabbed using get will be registered inside the facet and optimized to parent paths (['projects', 0] and ['projects', 1] becomes ['projects']). And now it starts listening/checking.
4. If a change is done to the now listening paths it will flag the facet as dirty and it will run again if something grabs the state
5. You set a new value on the facet using set(), and it becomes dirty

You might make an argument on conditional use of get, but that conditional would have to be triggered by either a change on current state paths listened to or changing the value itself, which will now run the reducer again and the new conditional path is registered.

Does this make any sense? The thing is that I would love to shorten the syntax of this stuff, just "do it", do not have to make sure that it listens to the right paths etc. It just works :)

[Yomguithereal]

This perfectly makes sense @christianalfoni. However I think we are pouring a lot of magic here. I fear it might get very costly and make the system work in some unexpected ways some times.

[christianalfoni]

Yeah, that is certainly a good point! I think I will make a branch on immutable-store, just to try it out. A fun project. I will let you know how it works out :-)

Currently the mapping functions in immutable-store, which are very similar to facets works very well. That being a library.makeMapping({}) or just a function does not really matter to me, but I think it makes sense to put it in the state tree itself. I acknowledge the arguments for having facets defined outside the tree and closer to the components that will use them, but in my opinion this is just state like any other state. Creating special concepts the components has to handle just increases complexity.

But yeah, I do not see any issue allowing both though.

[Yomguithereal]

I agree with you concerning the computed state within the tree. While having the computed state outside the tree has its advantages, it forces the components to "know" from which part of the state, static or dynamic, they should draw their deps and this is tiresome. This is even more tiresome when there are two different APIs to access somewhat similar things.

[Yomguithereal]

By the way, @christianalfoni, I just released [email protected] with updated dynamic data scheme: here. Will hopefully release the v2 very soon.

[Yomguithereal]

Let me know if you start experimenting on immutable-store so we can gather the underlying implementation problematics and tackle them together if you want.

[christianalfoni]

Ah, cool, hehe... monkeys... nice :-)

Thought anymore on how to import state? You mentioned merge, though will that not override the monkeys?

Yup, will let you know!

[Yomguithereal]

I often find myself merging the state as stored in the localStorage with the default state of the tree. The monkeys are just part of the default tree, in fact.

[christianalfoni]

But how does that work if a monkey is nested? Would not a merge at top level override the monkeys?

[Yomguithereal]

I use a deep merge. Like the lodash's one for instance.

[christianalfoni]

Ah, I see, so you deep merge the state object before you put it into the tree?

And if you want to do an import you simply grab the state object, deep merge and do a new "set" on the whole tree?

[tnrich]

Hey all and @Yomguithereal, sorry to interrupt the flow of conversation. I updated my my app to use the latest version of the baobab v2 branch and am now hitting errors due to the api changes discussed above, or at least I think that's what is causing it. I thought I'd double check with you as to what the new facet syntax is.

Here's the one version of the old syntax:

    $sequenceLength: [
        ['sequenceData'],
        function(sequenceData) {
            return sequenceData.sequence ? sequenceData.sequence.length : 0;
        }
    ],

Is the new syntax something like this?

    sequenceLength: Baobab.facet([
        ['sequenceData'],
        function(sequenceData) {
            return sequenceData.sequence ? sequenceData.sequence.length : 0;
        }
    ])

Thanks!

Also, moving away from the $ seems like a good move, be it to a pure function or a library method.

[Yomguithereal]

@christianalfoni that's it. I shall add soon a deepMerge method to the library anyway so that kind of things remain easier to perform. Generally however, I tend to deep merge all this before instantiating the tree rather than instance then set.

@tnrich, the only changed thing is indeed the computed data syntax. You have it right except that it's not Baobab.facet but rather Baobab.monkey or Baobab.dynamicNode. The full reference is available here.

[christianalfoni]

@Yomguithereal There seems to have been done great work here: https://www.npmjs.com/package/deepmerge. I am going to use that for the new import / export methods of the cerebral-baboab project. But yeah, would be nice to have it part of Baobab itself :-)

[Yomguithereal]

Lodash's merge or even jQuery's extend also work that way. On [email protected] you can even use Baobab.helpers.deepMerge.

[christianalfoni]

Ah, thanks for that, will use that one instead then :)

[Yomguithereal]

If you use it, please remember that my version is variadic and will mutate the first given object (like lodash). So you'll have to pass {} as the first argument if you don't want to mutate anything.

[christianalfoni]

Okay, cool, thanks for the heads up!

[Yomguithereal]

By the way, @christianalfoni, with cerebral, do you update with requestAnimationFrame or setTimeout 0? I am pondering this for #333.

[christianalfoni]

I use requestAnimationFrame when a signal is triggered and batch up any following signals. That way the signals has 16ms to run their logic and synchronously trigger a change event which will run the UI logic. And it is really between the start of a state change and an updated UI you want to give room for, and measure, the performance I think.

That said, if there is too much going on in regards of state change logic, it would make sense to trigger a new requestAnimationFrame after state change is finished and give UI update a new 16ms to do its thing. Still experimenting with this.

I would always use requestAnimationFrame over setTimeout as it gives you these predictable 16ms that will not affect UI rendering.

But I also give the opportunity to use someSignal.sync(data) to force synchronous updates all the way through... for inputs and stuff.

[christianalfoni]

And if the UI library already has requestAnimationFrame it does not matter because 100ms is the limit of noticeable delay. So an update could take in worst case like 50-60 ms including final rendering, in total, but it is jitter safe :)

At least, that is the theory! hehe