make context() a getter/setter

[jonschlinkert]

related to https://github.com/jonschlinkert/template/issues/16 and the context-manager branch.

@doowb I think we should at least consider making lvl a getter/setter. there is too much noise in template objects with the context stuff in the context-manager branch.

[doowb]

I think .context() should just be a method that allows passing in a key to get a property from the global context (like it is now). But I think we can add another parameter to allow passing in the array for using specific objects to merge. Basically making it short hand for doing this._context.calculate(['foo', 'bar']);

All the "setting" pieces for context should be done through the internal methods for each method. But... you're comment about lvl makes me think that when you say "setter" you mean setting the context objects and levels on the manager and not setting single properties. We could probably do that if you pass in an object:

var myContext = {foo: 'bar'};
template.context({
  name: 'my:context',
  level: 50,
  context: myContext
});

var foo = template.context('foo', ['env', 'my:context']);

Is this still too complicated?

[jonschlinkert]

Is this still too complicated?

yeah, I think so. I didn't think the context function would replace options and other built-in convenience methods that set/get on template.options. I thought it was only going to calculate the context at certain places at runtime. e.g. like a drop-in replacement for extend(), but with levels that are easier to define

[doowb]

Right now it's just calling calculate for the global pieces (env, defaults, options) and using get-value to be able to just get a specific value after the context is calculated. This is needed so we can see if a user has overridden an option without modifying the env and defaults.

We can remove the get-value part and just make it a getter that returns the calculated context.

var foo = template.context.foo;

[jonschlinkert]

We can remove the get-value part

I'm not sure yet, I guess it depends on total impact on speed. Anecdotally, before I changed the defaults and env to do simple get/set, the tests took twice as long to run. I'm not sure how much impact get-value is having. Honestly, on a related note I've been thinking about ripping a bunch of stuff out of config-cache because it's just too heavy atm.

[doowb]

I was thinking the same thing when I was trying to use config-cache to do enable/disable and it required "priming" the options object on env and defaults.

The slow piece is most likely because it's calling calculate each time a value was being asked for. I had started working on doing some caching but it would require listening for events (on config-cache to know when values are dirty. If we just build that in (in env() and defaults()) then it should be faster, but we would still need to watch for changes on options.

[jonschlinkert]

then it should be faster, but we would still need to watch for changes on options.

We have events on config-cache, should we do this the right want and setup listeners, like we were starting to do with assemble a while back?

[doowb]

I think we would need to add events to options-cache too.

[jonschlinkert]

are we using option-cache in this?

(edit) or somewhere in the tree?

[doowb]

config-cache inherits from option-cache and template inherits from config-cache.

[jonschlinkert]

hmm, or we can move that all back into template as an internal module. I created option-cache specifically to not have to use events. config-cache is too heavy as it is

[doowb]

I was going to suggest moving option-cache out of config-cache and also moving plasma out of config-cache. I don't think the events make anything "heavy"... it was when I tried to clone objects to pass to events before that make it really slow. Also, I had started a truly async event manager that is really simple and doesn't have the issues that we've run into with EventEmitter2.

[jonschlinkert]

The other thing I don't like about the events module that we're using is that it pollutes the root object with like a dozen properties that have no meaning to this lib.

I don't think the events make anything "heavy"

something makes config-cache noticably slow compared to option-cache. Seemed like events might be the reason, but it might be plasma (because of js-yaml specifically), but the refactor I just did might fix that. Both libs use get-value, so I don't think it's that.

[jonschlinkert]

most likely because it's calling calculate each time a value was being asked for.

@doowb I think this was the part I've been struggling with. I'm wondering if or why this is necessary. It seems that the only place where context should be calculated is at render time before context is passed to the engine. If we can get away with that, this means we only need this logic in the the render-related methods.

I think "keeping track" of options and data depending on where it's added is a completely separate operation that wouldn't do any kind of calculating and is fully autonomous and encapsulated, only acting as storage. Technically, I guess this "tracking" part could also be done by the context manager, but it doesn't even need to be the same instance of it since it could pass the objects to the context-manager to be calculated pre-render.

Hope this makes sense. it's also possible I'm forgetting why or completely misunderstanding the point of calculating in other places.

[jonschlinkert]

one part I forgot about was ensuring that users can override options/data. However, I still think that's a completely separate thing that is better handled with lazy loading

we can make this easier with methods for checking to see if an options key or value was defined:

if (!this.optionExists('foo')) {
  this.enable('foo');
}