yui/yui3 — pull request #917 by sdesai (2013-06-24 19:21:07+00:00)
Title: Add All Attrs in one-shot : Rollback or push forward

**This pull request is for discussion, more than merging at this point. However it contains working/built code.**

**It is a long one, but important to read if you're writing your own Extensions, or generally invested in the YUI component infrastructure layer. We have some time before Sprint 8 code complete, so don't need to rush it.**

During 3.11.0pr1 evaluation (thanks to everyone who did that) AlloyUI came across a real-world issue due to the Base order of operations changes introduced by https://github.com/yui/yui3/issues/629

I believe it's resolvable, without having to rollback the fix for the issue above, with the resolution leaving us in a better place going foward.

However, the resolution has a much larger probability of Extension developers having to upgrade their custom Extension impls, so I think it warrants broader YUI committer and user feedback, especially from people writing component Extensions.

Some details first, and I'll end with the choices on the table, for discussion/feedback.

# Details

For,

```
[A]~[ExtA]
|
[B]~[ExtB]

B extends A.
A mixes in ExtA.
B mixes in ExtB.
```

The order of operations, as it pertains to constructors, attr setup, and initializers, is described below:

## FLOW 1: Beginning of Time - 3.10.3

Manual chained, B Constructor -> A Constructor, kicks off:

1. ExtA Constructor
2. A ATTRS (valueFn, setters, getters, validators called)
3. A initializer
4. ExtA initializer
5. ExtB Constructor
6. B ATTRS (valueFn, setters, getters, validators called)
7. B initializer
8. ExtB initializer

### THE BUG HERE

* If B overrides setter/getters etc. for A's ATTRS, they can't access B's ATTRS, since B ATTRS are set up in the future.

This is the issue we were attempting to fix with the 3.11.0pr1 change to add all attributes in one shot. The bug, although not common, is present in the wild, and is difficult to grok for component developers.

## FLOW 2: 3.11.0 PR1

Manual chained, B Constructor -> A Constructor, kicks off:

1. ExtA Constructor
2. A + B ATTRS (valueFn, setters, getters, validators called)
3. A initializer
4. ExtA initializer
5. ExtB Constructor
6. B initializer
7. ExtB initializer

### BACK COMPAT ISSUE

* Existing **B ATTRS** may rely on **A initializer, ExtA initializer** having run. This was the expected low risk change, as documented. Noone seems to have hit this in real-world impls.
* Existing **B ATTRS** may rely on **ExtB constructor** having run.

The last issue was hit by AlloyUI in the PR. It's similar to the issue above, but maybe more common, since it's encapsulated in the same class. That is, it seems like it would be common for B's ATTRs to depend on ExtB's constructor.

## FLOW 3: Potential Fix? 3.11.0 - The End of Time.

Manual chained, B Constructor -> A Constructor, kicks off:

1. ExtA Constructor
2. ExtB Constructor
3. A + B ATTRS (valueFn, setters, getters, validators called)
4. A initializer
5. ExtA initializer
6. B initializer
7. ExtB initializer

### BACK COMPAT ISSUE

* Existing **B ATTRS** may rely on **A initializer, ExtA initializer** having run.
* Existing **ExtB Constructor** may rely on **A ATTRS, A initializer, ExtA initializer** having run.

We hit a set of older extension constructors in the library (WidgetPositionAlign, WidgetButtons, etc.) which did depend on the second issue - A's ATTRS having been set - and failed.

However all of these were older extensions, written before `initializer` support for extensions was added, so the constructor was the only place to stick this code. The intention was always to update these to use `initializer` when time/priorities permitted.

Once this was done, **all** library unit test code passed.

----

# Solution Options

**A. Roll back to FLOW 1**

PROS:

* No back compat breakage.

CONS:

* The bug mentioned, remains open.
* The bug/flow remains difficult to grok.

**B. Stick with FLOW 2**

PROS:

* Doesn't seem to be that common.

None of the library code had it. None of the other PR adoptors seem to have it. Alloy UI only hit it in their older deprecated components.

* Won't require folks to revisit their Extension implementations.

CONS:

* The order of operations, compared to FLOW3 is still messy to grok.
* Although not hit by anyone so far, logically, it seems like it would be a more common scenario (for B's ATTRS to depend on ExtB's constructor), so we may have to do this again post GA if this proves to be true.

**C. Push on to FLOW 3**

PROS:

* This seems like the most grokable, maintainable flow moving forward.

CONS:

* There is a strong possibility that Extension developers (outside of library code, which we'll take care of), will need to move code from their Extension's constructors to their Extension's `initializers` as we had to do for the older out of the box Extensions in this pull request.

----

# My Take

* I'd like to move to Flow 3

* It's the cleanest story going forward.
* However, we can't force that change without some level of general YUI community consensus, due to the potentially wider upgrade impact to Extension developers.

* I don't think we can stick with Flow 2 for 3.11.0.

* I think it's not going to be as uncommon as unit testing/PR adoption suggest, and I'm hesistent to do two rounds of this, if it does turn out there are more occurrences in the wild, post GA.
* It should be noted that the performance improvements in 3.11.0pr1, won't be impacted if we roll back. Most of that comes from the event bypass.

* In the absence of feedback/general consensus, I'll roll back to Flow 1 for Sprint 8, and pick this up again later.

**NOTE:** While providing feedback don't let my take influence your opinions. That is, I'd be perfectly fine to roll back the change also, and revisit this again in another sprint. I'm just trying to gauge opinion on the benefits vs. potential upgrade impact of Flow 3, in the sprint 8 timeframe.

If you'd like to see what impact Flow 3 would have on your code bases, this pull request/branch should give you a way to try it out (hence the build files committed).

It's not written for performance at this point (has one more loop around classes than I'd like), but the flow/functionality is the expected final functionality.