Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

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

Closed
wants to merge 5 commits into from

4 participants

@diervo

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

@mojit0

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

Sounds good to me so far! xDD

@ricallinson

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
Owner

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

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
Owner

@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

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

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

@diervo diervo closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
1  lib/app/autoload/mojit-proxy.client.js
@@ -172,6 +172,7 @@ YUI.add('mojito-mojit-proxy', function(Y, NAME) {
instance = {
base: this._base,
type: this.type,
+ action: action,
guid: this._instanceId, // DEPRECATED, use instanceId instead
instanceId: this._instanceId,
config: Y.mojito.util.copy(this.config)
View
8 lib/app/autoload/resource-store-adapter.common.js
@@ -61,23 +61,23 @@ YUI.add('mojito-resource-store-adapter', function(Y, NAME) {
// DEPRECATED, but kept in case a user is using.
instance.guid = instance.instanceId;
+ source.action = instance.action;
// What are being asked to expand?
if (instance.base) {
- source.name = instance.base;
+ source.base = source.name = instance.base;
source.func = this.getSpec;
} else if (instance.type) {
- source.name = instance.type;
+ source.type = source.name = instance.type;
source.func = this.getType;
} else {
// We don't have any inputs so fail
throw new Error('There was no info in the "instance" object');
}
-
// This contains the app "definition" and app config
my.getApp(env, context, function(app) {
// Here we get either the a spec or a type
- source.func(env, source.name, context, function(err, data) {
+ source.func(env, source, context, function(err, data) {
if (err) {
callback(err, {});
return;
View
10 lib/app/autoload/store.client.js
@@ -135,9 +135,10 @@ YUI.add('mojito-client-store', function(Y, NAME) {
/*
* TODO: REVIEW RE [Issue 76].
*/
- getSpec: function(env, id, context, callback) {
+ getSpec: function(env, spec, context, callback) {
- var parts = id.split(':'),
+ var id = spec.name,
+ parts = id.split(':'),
typeName = parts[0],
specName = parts[1] || 'default',
ns = typeName.replace(/\./g, '_'),
@@ -166,13 +167,14 @@ YUI.add('mojito-client-store', function(Y, NAME) {
/*
* TODO: REVIEW RE [Issue 77]
*/
- getType: function(env, type, context, callback) {
+ getType: function(env, spec, context, callback) {
// This should really have the tunnelPrefix. However, that
// complicates offline apps (from `mojito build html5app`).
// The mojito-handler-tunnel will be able to handle this URL
// just fine.
- var url = this.staticPrefix + '/' + type + '/definition.json';
+ var type = spec.name,
+ url = this.staticPrefix + '/' + type + '/definition.json';
url = this.buildUrl(url, context);
View
6 lib/store.server.js
@@ -475,7 +475,8 @@ YUI.add('mojito-resource-store', function(Y, NAME) {
* @param {function(err,spec)} callback callback used to return the results (or error)
*/
getSpec: function(env, id, ctx, callback) {
- this.expandInstanceForEnv(env, {base: id}, ctx, function(err, obj) {
+ var base = Y.Lang.isString(id) ? {base: id} : id;
+ this.expandInstanceForEnv(env, base, ctx, function(err, obj) {
if (err) {
callback(err);
return;
@@ -499,7 +500,8 @@ YUI.add('mojito-resource-store', function(Y, NAME) {
* @param {function(err,spec)} callback callback used to return the results (or error)
*/
getType: function(env, type, ctx, callback) {
- this.expandInstanceForEnv(env, {type: type}, ctx, function(err, obj) {
+ var typeObj = Y.Lang.isString(type) ? {type: type} : type;
+ this.expandInstanceForEnv(env, typeObj, ctx, function(err, obj) {
if (err) {
callback(err);
return;
Something went wrong with that request. Please try again.