bound the right context to the aftersubscriber of _afterModelChange #1498

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@ItsAsbreuk

Making 'this' refer to the current LML-instance (inside _afterModelChange)

@rgrove
rgrove commented Dec 16, 2013

@ItsAsbreuk Do you have a failing test case that demonstrates why this change is necessary?

In reviewing your change I looked at the existing unit tests. We already seem to have a test that covers this case, and it's passing before your change. I checked the value of this manually just to be sure, and it is indeed set to the LazyModelList instance inside _afterModelChange() even before your change. Am I missing something?

@ItsAsbreuk

Well, you see at the code that the context isn't passed through.
I must have ran into a situation where the context changed. I don't know what made this.

I do know that a js-error occured at https://github.com/yui/yui3/blob/master/src/app/js/lazy-model-list.js#l455 saying "this._clientIdMap is undefined". Once I made the codefix (i can because we serve trhough our own comboloader), the issue was solved.

I need to isolate my situation, but that takes some time: it appeared in a complex system with multiple modules. But i'm in a time-problem right now, so it may take some time before i get back to your question. sorry for that.

Perhaps you can see a reason what could make the context to change? targeting, bubbled change-events, class-extention?

@rgrove
rgrove commented Dec 16, 2013

I'm not sure what happened in your case, but the event system seems to be preserving the context, which is what I'd expect from this.after().

@ItsAsbreuk

btw, the LML is an attribute inside a Widget. maybe there the eventsystem disturbs something, i don't know.
I'll isolate my situation and come back later on.

@ItsAsbreuk

I think it might have to do with promises.
For I am using a synclayer based on promises and promises loose context inside the thennable.

I wasn't able to reproduce it with a simple custommade setup.
So i need to strip a bunch of code inside multiple modules to find out what exactly caused this.
It might take some time before i can pick this up.

@triptych

@ItsAsbreuk should this be marked "[WIP]" or perhaps closed out until you get a chance to take a look at this again?

@rgrove
rgrove commented Apr 16, 2014

@triptych I'm gonna close this. It's a problem in @ItsAsbreuk's code, not the library.

@rgrove rgrove closed this Apr 16, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment