feature: models are optional for perf reasons #500

Merged
merged 5 commits into from Sep 13, 2012

Conversation

Projects
None yet
2 participants
Collaborator

caridy commented Sep 13, 2012

you should require model mojito-models-addon in your controller to access the default list of models through ac.models.FooModelFoo just like it was before.

@caridy caridy feature: models are optional now, you should require model mojito-mod…
…els-addon in your controller to access the default list of models through ac.models.modeName
d8cf4b0
Member

drewfish commented Sep 13, 2012

I think the idea was to have a getter instead, as in ac.models.get('foo') to return the model.

@drewfish drewfish commented on an outdated diff Sep 13, 2012

lib/app/addons/ac/models.common.js
+YUI.add('mojito-models-addon', function (Y, NAME) {
+
+ 'use strict';
+
+ /**
+ * <strong>Access point:</strong> <em>ac.models.*</em>
+ * Addon that provides access to the models collection
+ * @class Models.common
+ */
+ function Addon(command) {
+
+ var instance = command.instance,
+ config = instance.config;
+
+ // making every model accessible through this addon
+ Y.Object.each(Y.mojito.models, function (model, modelName) {
@drewfish

drewfish Sep 13, 2012

Member

Using Y.mojito.models as the source isn't correct. We should be using instance.models instead.

Collaborator

caridy commented Sep 13, 2012

  • yes, using ac.models.get('modelName') will be better in terms of performance, building models on demand, but will break BC.
  • yes, iterating on instance.models should be better, I wasn't sure due to the old code where all models were included if shareYUIInstance was false.

Anyway, let's break the BC and we will see :p

Member

drewfish commented Sep 13, 2012

The config AC addon will be breaking BC as well (and it has to).

caridy added some commits Sep 13, 2012

@caridy caridy models addon provides models on demand rather than instantiating all …
…of them at once. This break AC. You can access models by doing ac.models.get('modelName').
9be09d8
@caridy caridy Merge branch 'search-perf' of git://github.com/yahoo/mojito into mode…
…ls-addon
623bdad

@drewfish drewfish commented on the diff Sep 13, 2012

lib/app/addons/ac/models.common.js
+ /**
+ * Gets model instance
+ * @method get
+ * @param {string} modelName The name of the model.
+ * @return {object} model instance, or null.
+ */
+ // this is an experiment where "get" method uses the closure
+ // rather than be directly attached, to avoid storing
+ // a instance.config or models reference in the addon instance.
+ this.get = Y.bind(function (config, modelName) {
+
+ var modelInstance;
+
+ // instantanting the model once during the lifetime of
+ // the ac object, this acts like an internal cache.
+ if (Y.mojito.models[modelName] && !models[modelName]) {
@drewfish

drewfish Sep 13, 2012

Member

I think the Y.mojito.models lookup is wrong. When using sharedYUIInstance, that will contain -all- the models in the app (including those in other mojits). Instead, the check should use instance.models[modelName].

@drewfish drewfish commented on an outdated diff Sep 13, 2012

lib/app/addons/ac/models.common.js
+ */
+ // this is an experiment where "get" method uses the closure
+ // rather than be directly attached, to avoid storing
+ // a instance.config or models reference in the addon instance.
+ this.get = Y.bind(function (config, modelName) {
+
+ var modelInstance;
+
+ // instantanting the model once during the lifetime of
+ // the ac object, this acts like an internal cache.
+ if (Y.mojito.models[modelName] && !models[modelName]) {
+
+ // TODO: Why? There's no particular reason to inherit here.
+ // @caridy: we have to, otherwise this.something in the model
+ // instance can be polluted.
+ modelInstance = Y.mojito.util.heir(Y.mojito.models[modelName]);
@drewfish

drewfish Sep 13, 2012

Member

Ideally this discussion would be cooked down into a single comment, something like:

// We have to heir() otherwise this.something in the model
// will pollute other instances of the model.

drewfish merged commit 340ae88 into yahoo:search-perf Sep 13, 2012

1 check failed

default The Travis build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment