dispatchToParents problem

[mmikeyy]

Hey all... Not a big problem but still.

We allow propagation to be stopped by extending the payload with {propagationDisabled:true }. Problem is that all events dispatched do not have a payload attached to them. Events without a payload cannot have their propagation stopped.

Solution is to always replace undefined payload with an empty object, all simply. I can't see any negative side effect.

Also, there seems to be an inconsistency in the treatment of the payload between dispatch, dispatchToParent and dispatchToParents.

The first one checks that the payload is an object, if defined:

        dispatch: function dispatch(eventName, eventData) {
            if (!_.isUndefined(eventData) && !_.isObject(eventData)) {
                throw "Event payload must be an object";
            }
            eventData = eventData || {};
            eventData.eventName = eventName;
            this.vent.trigger(eventName, eventData);
        },

Ahh! I hadn't noticed but here we see the payload set to an empty object by default!!!

The second one does no checking at all on the payload and just triggers the event on the target with whatever is supplied as payload:

           dispatchToParent: function dispatchToParent(eventName, eventData) {
            if (this.parentContext) {
                this.parentContext.vent.trigger(eventName, eventData);
            }
        },

For dispatchToParent, instead of

this.parentContext.vent.trigger(eventName, eventData);

we should have

 this.parentContext.dispatch(eventName, eventData);

and dispatchToParents could be fixed the same way.

Another benefit is that eventName would be added to payload in all cases, and not just for one of the three flavors (improved consistency)

Let me know if I'm missing something.

Let me know also if you want me to do PR, tests et all.

PS congratulations @creynders on your recent anointment as knight of the Geppetto realm! BTW, could you put on your shining armor and quickly explain something to me? I forked the repository at a certain point in time and then branched from it for the PR's that I made. That worked well. But now that the main repository has moved to 0.7.1, I find my fork to be outdated and I've been looking all over the user interface for a way to simply reset it to be equal to the main repository. Can't find anything. Is one supposed to delete his fork and fork again? (or ... should I just RTFM?)

[creynders]

Good catch!

Is one supposed to delete his fork and fork again?

No, you can add this repo as an extra remote. Traditionally it's called upstream. So, in this case you would add it like this:

git add remote upstream git@github.com:GeppettoJS/backbone.geppetto.git

Now all you have to do is checkout master and

git pull upstream master

Tip: you can see your remotes and what urls they point at with

git remote -v

There's another git-fu technique you should know about, now that you've entered OSS contributors-land:

As you probably noticed there were a lot of merge-commits in your previous PR. Many project maintainers frown upon these since they create merge-bubbles, like this:

This happens when you use merge to update your branches. It's best to use rebase instead, since this will play back your changes (i.e. commits) on top of the changes you want merged.

For example: let's say your local and upstream master are in sync at a certain commit. You create a branch patch-1 and start working on it, committing your changes. You issue a pull request. However this takes a few days and in the mean time the upstream master branch has been updated with new changes. And you notice you need to add some changes to patch-1. If you pull the upstream changes into your master branch and merge patch-1 with it (or pull the upstream changes directly into patch-1) you'll have created a merge bubble. It's better to checkout your master branch, pull the changes from upstream and subsequently checkout patch-1 and do:

git rebase master

This will take the commits in patch-1 and apply them on top of all commits in master, thus keeping git history flat. Now you can add the changes you want to patch-1

However if you try to push origin patch-1 Git will complain about non-fast forwarding and bail out on you. This is because the history as recorded in your patch-1 branch in origin deviates from the history of your local patch-1 branch. The solution to this is to force the push with

git push origin patch-1 -f

Github is smart enough to recognise what you did and will update your PR accordingly, i.e. the (outdated) commits it showed will disappear and will be replaced with the new ones. And if your PR is merged in, everybody will still have a flat git history.

I know, all of this sounds a bit overwhelming when you're not used to it. I found the best way to learn git through-and-through is to always visualise what you're trying to do/doing. There's an excellent site to help you understand what happens when merging, branching, rebasing et cetera: Learn git branching It starts out easy, but gets more complex pretty quickly.

[mmikeyy]

wow! the armor is so shiny I have to squint my eyes!

I understand the need to rebase now. I had the impression I was kind of shooting at a moving target when I was doing my PRs after merging repeatedly on a repository that had been changed by others in the meantime My merge bubbles do look messy...

The main obstacle to progress here is ... fear. I dread the moment I get a cryptic GIT message telling me that somehow, several months' work have just sunk to the unfathomable depths of an abyss (I do have backups, but I like to see them as protection against accidents, not against my ignorance...). So I use a limited set of functions that work for me, but are obviously not sufficient in a collaborative context.

So thanks for the tips, and for your time. I will spend some time improving my GIT proficiency as soon as I have a chance. Arrghhh... who decided there were to be only 24 hours in a day, and why is life so damn short??

[creynders]

The main obstacle to progress here is ... fear.

Your fear is not totally unwarranted. Git can go horrible wrong. However, everything that's pushed to remote is safe, i.e. in a worst case scenario you re-clone and pull everything in. I also used to make a manual copy of the entire project folder (including the .git directory) right before doing complex shizzle. Old skool, but better safe than sorry.

[creynders]

@mmikeyy are you going to tackle this problem? No worries if you don't have the time, just wondering. Since in that case I'll probably fix this myself.

[mmikeyy]

Yeah... sorry for the delay: I've been a bit overwhelmed lately. I'm going to fix this now.

[creynders]

@Mmikeyy seriously, no rush.

[mmikeyy]

OK thanks, but I did spend the time finally. Time I don't even have! Arrghh!

You may look at my fork in branch gh-pages2. It's all there. Only two files changed: geppetto-specs.js and backbone.geppetto.js.

To summarize, I did the following:

added a method _extendEventData to Geppetto.Context that is called by dispatch to do the same thing as before (check that payload is object, extend with eventName);

Pulled that out of the dispatch method so it can be reused in dispatchToParents without duplicating code.

Changed the way dispatchToParentsworks. It used to call itself recursively. But now that we check and extend the payload, we're not going to do this again and again with each call. So the payload check/extension is done once when the function is called, and a while loop loops through the ancestry afterwards.

Tests have been upgraded accordingly.

Lastly, I've tried a few things with git but it's a total failure and I don't have time to figure things out right now. If I do a pull request on the branch mentioned above, I see an impossibly large number of file changes made by others that would be included in the PR. So I prefer to abstain rather than imposing such a mess on others.

@creynders, would it be possible for you to work your magic and do the PR?? I just designated you as a collaborator on my fork ( or anointed you as a knight, if you prefer!).

Side note: my changes pass the tests with flying colors locally, but I would normally feel better if I could see the thing working with an actual program. I've been spending weeks converting a big program to AMD and Geppetto, and the switch to 0.7.1 from 0.7.0 broke everything. I have to stick with 0.7.0 until I figure out what it is (and it seems to be more than just one thing) that worked before and no longer does. Anyway, not writing this to complain, just to stress that the tests are the only tests that could be performed.

[creynders]

I see an impossibly large number of file changes made by others that would be included in the PR.

Could it be you created a branch of gh-pages? It's a special branch specifically meant to update github pages You should never use it as a source for a branch, nor push to it. (Unless you want to update the Geppetto site)

the switch to 0.7.1 from 0.7.0 broke everything

Ouch. That shouldn't be happening. 0.7.1 should be backwards compatible, that's why it was a patch release, not a minor one. That's bad news.

[creynders]

In case you wonder what I did:

#
# retrieve your changes, without applying them to my current branch
#
git fetch mmikeyy gh-pages2
#
# view git history in order to get the SHA-ID's of the commits I need
#
git log mmikeyy/gh-pages2
#
# create a new branch 'fix_72'
#
git checkout -b fix_72
#
# cherry-pick your changes
#
git cherry-pick 6655888e1767d1e3968083e750270d786a329067
git cherry-pick 0d9703c9e3cf04b731c252f771b7d999c7195f7c
#
# rebase interactively, to rewrite git history. I "squashed" your two commits into one
#
git rebase -i HEAD~2
#
# push to the "fix_72" branch in your fork
#
git push mmikeyy fix_72
#
# turn issue 72 into a pull request
# directed from "fix_72" of your fork towards "master" of the official repo
#
hub pull-request -i 72 -b geppettojs/backbone.geppetto:master -h mmikeyy/backbone.geppetto:fix_72

[mmikeyy]

Could it be you created a branch of gh-pages?

Yep. That's what I did. :flushed:

Ouch. That shouldn't be happening.

I know... But then, I might have been doing a few things in an unorthodox manner that just worked. When I revisit the parts of the app that were converted to Geppetto first, I sometimes stumble upon things that make me cringe now. In any event, I'll have to figure out what's going on and I'll see then if I find anything worth mentioning here.

Thx for cleaning up my mess! :flushed: Will examine your process when I have time. Another crazy day ahead so it might not be today...

[mmikeyy]

Ouch. That shouldn't be happening. 0.7.1 should be backwards compatible, that's why it was a patch release, not a minor one. That's bad news.

OK. I've got the beginning of an answer (i.e. why 0.7.1 is breaking a working 0.7.0 application).

in 0.7.0, the Context constructor contained these lines (around 194 and following):

        if (_.isFunction(this.initialize)) {
            this.initialize.apply(this, arguments);
        }
        this._contextId = _.uniqueId("Context");
        contexts[this._contextId] = this;

        var wiring = this.wiring || this.options.wiring;
        if (wiring) {
            this._configureWirings(wiring);
        }

In 0.7.1, the exact same lines become:

        this._contextId = _.uniqueId("Context");
        contexts[this._contextId] = this;

        var wiring = this.wiring || this.options.wiring;
        if (wiring) {
            this._configureWirings(wiring);
        }
        if (_.isFunction(this.initialize)) {
            this.initialize.apply(this, arguments);
        }

I was in the habit of doing a few adjustments to @wiring in the initialize method after I realized that the wiring property of the prototype was shared by all instances, which can easily go unnoticed and cause unexpected problems, in particular with values.

Anyway, I assigned values to the wiring property in initialize so these values would belong to the context instance, and then these changes were processed by this._configureWirings(wiring);, which was called at the very end of the constructor, after initialize was called.

Now, with 0.7.1, this._configureWirings(wiring) is called before this.initialize.apply(this, arguments).

This apparently insignificant change breaks my application because now, changes to wiring performed in initialize remain unprocessed.

Perhaps there's a good reason for changing the order of these statements. Perhaps it's accidental. But thinking of it, I suspect it's a good idea because the initialize method is now executed in a context that is all wired up. And all one has to do is use wireValue, wireCommand etc to add extra wiring.

I may have missed some discussions about this change in the order of these lines. But if the change is accidental, would it not be a good idea to add a test to ensure that they are not changed again?

For the moment, I put these lines back in the original order and my app is almost fixed. Now, I have a problem with a view class that is sent to a layout that instantiates it and renders it in a region. That used to work well with 0.7.0. Now, with 0.7.1, instead of receiving a view class to instantiate, the layout receives:

function () {
            return applyToConstructor(clazz, _.toArray(arguments));
        } 

I'm now going to try and figure out why this is happening...

[creynders]

@mmikeyy as you can see I turned your last comment into a separate issue, since it hasn't got anything to do with dispatchToParents. I'll expound our reasoning for those changes there. (And why we thought we could get away with them w/o breaking anybody's code.)