Skip to content

BaseObservable — Extracting lifecycle and attribute change observability from Y.Base#168

Closed
ericf wants to merge 20 commits into
yui:masterfrom
ericf:base-events
Closed

BaseObservable — Extracting lifecycle and attribute change observability from Y.Base#168
ericf wants to merge 20 commits into
yui:masterfrom
ericf:base-events

Conversation

@ericf

@ericf ericf commented Jun 22, 2012

Copy link
Copy Markdown
Member

This extracts Y.BaseObservable, an extension for Y.BaseCore, from Y.Base.

By extracting the lifecycle and attribute change observability features, BaseCore subclasses have the potential to mix these features back in at runtime. These changes (and probably more) are needed to create the Y.Model.Base class and Y.Model.Observable extension.

I ran the base unit test and they all pass; additionally, I ran app's and a few other higher-level components to make sure I didn't introduce any gaps or regressions.

Running Locally

I explicitly did not commit the build files or the Loader metadata update, when trying out these changes locally, do the following:

$ cd src/attribute/ && shifter
$ cd ../base/ && shifter
$ cd ../yui/ && shifter

Todos

There are several things that still have to be done before this is merged in. Many of these came up in a code review of this Pull Request.

  • Determine if `Y.BaseObservable` can be mixed into a `Y.BaseCore` subclass at runtime.

  • Try implementing `Y.Model.Base` and `Y.Model.Observable` on this structure.

  • Move Attribute's `_protecteAttrs()` prototype method to a static utility method on the `Y.Attribute` constructor function.

  • Have `Y.BaseObservable` not mix `Y.Attribute` as a whole, instead:

    -

    `Y.Base` will itself mix-in `Y.AttributeExtras` before `Y.BaseObservable`.

    -

    Investigate whether `Y.Model.Base` should itself mix-in `Y.AttributeExtras`.

  • Remove `Base._buildCfg = ` from `base-build`:

    -

    Create a separate object used for Base's config that's a copy of BaseCore's.

  • Move the `_ATTR_CFG_HASH` computation into `base-build`.

  • Naming: I kind of like "observable" better, e.g. `Y.BaseCore` and `Y.BaseObservable`. This should trickle down to `Y.AttributeObservable` too.

  • Figure out correct yuidoc tags to use to make the API docs right (possibly a bug in yuidocjs).

ericf added 3 commits June 15, 2012 15:29
Move the observablility of `Y.Base`'s lifecycle and attribute changes
into `Y.BaseEvents`, an extension for `Y.BaseCore`.
Comment thread src/base/js/Base.js Outdated

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having this method on Y.Base may affect the ability to mix in Y.BaseEvents into a Y.BaseCore subclass at runtime.

@ericf

ericf commented Jun 22, 2012

Copy link
Copy Markdown
Member Author

@sdesai This is the initial work I've done to extract Y.BaseEvents from Y.Base. There is still more I want to do to make sure this services the needs of Y.Model.Base and Y.Model.Events. Let me know if anything stands out to you…

@lsmith

lsmith commented Jun 22, 2012

Copy link
Copy Markdown
Contributor

I wonder if this is similar to the DT class extensions, in that I included @for DataTable.Base in the docs so the methods would show up where they were most likely to be searched for. The module description for the class extensions include a note that the API docs are in DataTable, since otherwise they appear empty. (e.g. http://yuilibrary.com/yui/docs/api/classes/DataTable.Mutable.html)

@ericf

ericf commented Jun 22, 2012

Copy link
Copy Markdown
Member Author

@lsmith Yup, this is the same thing I'm seeing. I guess we can sort that out when @davglass get's back and determine if there's something we want yuidoc to do differently or not.

Comment thread src/base/js/Base.js Outdated

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure _ATTR_CFG and _ATTR_CFG_HASH should be aggregated by Y.Base.create() and Y.Base.mix(). Looking at Y.Base._buildCfg, I see the following:

Base._buildCfg = {
    custom: {
        ATTRS: function (prop, r, s) { /* ... */ },
        _NON_ATTRS_CFG: arrayAggregator
    }
};

It seems natural to add _ATTR_CFG and possibly _ATTR_CFG_HASH to this set. This way, you could mix Y.Base.Events (which wants to add things to _ATTR_CFG) to a Y.Base.Core subclass at runtime via Y.Base.mix().

Since _ATTR_CFG_HASH depends on _ATTR_CFG being fully aggregated, it would be brittle to rely on object property order for its function to be called after _ATTR_CFG's :-/

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation I ended up with is in d92409c

This allows BaseCore to be the root Base-based superclass enabling the
ability to mix in BaseEvents at runtime. The following is now possible:

    Y.Foo = Y.Base.create('foo', Y.BaseCore, []);

    // And...

    Y.Bar = Y.Base.create('bar', Y.BaseCore, [Y.BaseEvents]);

    // Or...

    Y.Baz = Y.Base.create('baz', Y.BaseCore, []);
    Y.Base.mix(Y.Baz, [Y.BaseEvents]);
@ericf

ericf commented Aug 30, 2012

Copy link
Copy Markdown
Member Author

@sdesai I'd like to get this in 3.x, but would like you to review it first. This is a precursor to #2532429, and @caridy might want to use the new capabilities this provides.

@caridy

caridy commented Aug 31, 2012

Copy link
Copy Markdown
Member

@sdesai the idea is to try to use this feature on the new mojito prototype we are building during the current cycle :)

@sdesai

sdesai commented Sep 4, 2012

Copy link
Copy Markdown
Contributor

Will take a look this week.

@sdesai

sdesai commented Sep 20, 2012

Copy link
Copy Markdown
Contributor

Sorry about the delay looking into this. I'm just going to put it in the next sprint, so it gets done.

@sdesai

sdesai commented Sep 24, 2012

Copy link
Copy Markdown
Contributor

Conflicts:
	src/base/base-base.properties
	src/base/build.xml
	src/base/tests/base-tests.js
@ericf

ericf commented Oct 8, 2012

Copy link
Copy Markdown
Member Author

Updated the Todos list in the Pull Request description based on feedback from @sdesai when we had a code review.

ericf added 10 commits October 9, 2012 17:52
This moves the protected `_protectAttrs()` method to a static utility
method on `Y.AttributeCore` and `Y.Attribute`.
The reduces dependencies of BaseEvents, but does introduce the fact that
mixing BaseEvents into BaseCore does not equal the functionality of
Base. This is because Base now explicitly mixes in AttributeExtras.
A Base/BaseCore-based class will now compute its `_ATTR_CFG_HASH` on
demand, when it is needed. If an extension is mixed in via
`Y.Base.mix()`, then the cached hash will be nulled-out.
This renames:

* Y.AttributeEvents -> Y.AttributeObservable
* attribute-events -> attribute-observable
This renames:

* Y.BaseEvents -> Y.BaseObservable
* base-events -> base-observable
@ericf

ericf commented Oct 12, 2012

Copy link
Copy Markdown
Member Author

Note: Travis is lying, this does actually pass all tests, I just decided against checking in the build files for the sake of the diff.

@davglass

Copy link
Copy Markdown
Member

I'm not sure why my scripts aren't picking it up, I'll ping the Travis
guys to make sure the Env is getting set.

@ericf

ericf commented Oct 22, 2012

Copy link
Copy Markdown
Member Author

It has been determined that the API documentation anomalies are issues with YUIDoc that will be reported and fixed there.

@ericf

ericf commented Oct 22, 2012

Copy link
Copy Markdown
Member Author

I am going to merge this into the 3.x branch today. /cc @sdesai

@ericf

ericf commented Oct 22, 2012

Copy link
Copy Markdown
Member Author

This has been merged in 3.x.

@ericf ericf closed this Oct 22, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants