Added action param to expandInstance for client/server (& tunnel indirectly) #530

wants to merge 5 commits into


None yet
4 participants

diervo commented Sep 19, 2012

Shaker need to know the action when expanding an Instance to serve the assets :)


mojit0 commented Sep 21, 2012

I'd still like a comment such as:

// This is only here because shaker needs it and we can't find a better way at the moment.



diervo commented Sep 21, 2012

Sounds good to me so far! xDD


ricallinson commented Oct 18, 2012

After discussing this in detail with the Shaker folks I now understand the general use-case of wanting the "action" to be part of the instance schema. Although Mojito core does not use it today anyone extending the Resource Store with an Addon could depend on the "action" for caching purposes or for applying specializations to the returned instance object. Not having the the "action" available limits the extensibility of mojito.

However, I'm not sure the new "definition.json" URL format is ideal.


caridy commented Oct 24, 2012

I see few issues on this PR:

  • semantically, "expandInstance" as we have it today, does not reflect the need of an action, actions are properties of the controller, not the mojit instance.
  • what was a simple request before (e.g: definition.json) is now a more complex request with parameters, which of course will not work in html5 and offline apps where those querystring parameters mean nothing.
  • for the sake of few mojits that might have completely different (octagonal) actions we will be sacrificing all other mojits that will probably use the same or similar binders. When I say sacrificing, I mean we will have a performance penalty whenever the same mojit is used with different actions but also adding a lot of complexity to bind the instance to the action.
  • this doesn't play nicely with the new feature introduced in 0.4.0 to support custom binder definition within the action, something like: ac.done({}, {view: {name: 'foo', binder: 'bar'}}), shaker will not be able to know about this during build time.
  • Since shaker will not control the JS dependencies, and rollups anymore, only the CSS, this really boils down to selecting the proper "skin/rollup" per action, and that, in production, might be a unique url per view.

My proposal is:

  • expand instances without action.
  • shaker can add css urls (single one in production per view). we already have such as structure defined as part of the expanded instance.
  • duplication of urls for similar views should be fine since gzip will help here, or we can introduce a wildcard like '*' to decouple skin/rollup from the actual view in case all actions use the same css.

With respect to "extensibility concerns" from @ricallinson, I'm not so sure about it, and I have concerns about the semantic of the current structures (even though we can change that semantic at any time), but what I'm very sure about is that performance is paramount, and having the ability to do more stuff (making more decisions) during the runtime process to expand the instance is a performance problems. In theory, RS addons will do a pretty dummy process IF they really need to expand the instance more. In other words, making this more restrictive will help to keep performance controlled.

/cc @jenny


diervo commented Oct 24, 2012

First of all this PR is out to date since was before the @drewfish refactor of the store couple of weeks ago.
Add css urls on the fly using combo load is not a path I want to take for css.
So I dont envision a better place than expand instance to be able to populate assets (because this will work no matter client or server).

At the end of the day, Shaker needs the context, the Type of the mojit and the action. As far as I have another place to take it and it will work on the client or the server, its fine for me.


caridy commented Oct 24, 2012

@dferreiroval, I'm not saying that u should not produce the proper css during expand instance, in fact, I'm saying you should, I'm just saying that you should not rely on the action.

About the context, the context is not based on the route, so, making a call to load definition.json will have the proper context, so, you do have the context without need to pass it as querystring.


diervo commented Oct 24, 2012

Again, the context is not enough. I need to have the action of the mojit being executed. This is a requirement for many teams, so Shaker cannot work properly without knowing which action-rollup to serve.
Take a look at the metadata ans she how rollups are distributed.


diervo commented Nov 13, 2012

I guess we dont need this anymore since we are attaching the css into the views

@diervo diervo closed this Nov 13, 2012

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