zachsnow / ng-multi-transclude

ng-multi-transclude
MIT License
79 stars 19 forks source link

Losing prototypical inheritance from controller scope #23

Closed rainboxx closed 9 years ago

rainboxx commented 9 years ago

Originally thought to be affected by #19 it seems this is a different issue. However, #16 seems to be similar.

See http://jsfiddle.net/8wt7ptmw/3 for an example.

With a normal ng-transclude I'm able to access the controller's scope though prototypical inheritance. Using ng-multi-transclude this prototypical inheritance gets lost. I am, however, able to access the controller's scope by using $parent.$parent, which is ugly and not obvious.

16 seems to be similar because in the example http://plnkr.co/edit/lv7AHh?p=preview made by @zachsnow (see comment in #16) there's the part captioned as "Doesn't work" which also doesn't work, but based on another comment in #16:

multi-transcluded blocks are linked in the context in which they are defined, not in context into which they are "injected"

I'd expect the example from #16 and mine to work.

rainboxx commented 9 years ago

The proposed Pull Request fixes both issues. See updated jsfiddle and forked/updated Plunker.

However, I don't know the reason for the childScope to be set here.


Links: http://jsfiddle.net/8wt7ptmw/4 http://plnkr.co/edit/E9RyXLo9o7SW2IkZHyaa?p=preview

worldspawn commented 9 years ago

Did this just happen? I came to work today and my transcluded content yesterday could read from the outer scope and this morning it cannot. Day ruined :(

--edit-- I must have reinstalled my bower dependencies and got 0.1.4 which broke. rolled back 0.1.3. need some tests on scopin'!

zachsnow commented 9 years ago

@worldspawn really sorry to have caused you trouble, multi-transclude definitely is still finding its feet. @rainboxx thanks for the pull request, I'll have a look in the morning and try to issue a new release. Thanks to you both for keeping an eye this.

worldspawn commented 9 years ago

No worries Zach. I an do a PR that gives this repo a bit more scaffolding (gulp, jshint, karma etc). Maybe a demo page too

zachsnow commented 9 years ago

@worldspawn that would be great -- this library has aspects of things I use in production but due to legacy requirements isn't in use at my primary project, so it doesn't get as much action as it might. Those things are definitely a step in the right direction!

zachsnow commented 9 years ago

@worldspawn @rainboxx apologies for the delay, I will have time to address this over the weekend.

rainboxx commented 9 years ago

@zachsnow Could you take a look at the issue? Can I help checking something? Thanks :-).

zachsnow commented 9 years ago

Unfortunately work got in the way. Re: pull request #24 it makes sense that we are using the wrong scope, and the fix seems good. It would be good to test a few of the issues we've been trying to fix, but since it's so broken I will probably merge immediately.

rainboxx commented 9 years ago

At least it fixes #16 and this one :-). Let me know if I can help...

zachsnow commented 9 years ago

@rainboxx so I tested it issues #14 and #18 seem fine using this. However, in https://github.com/zachsnow/ng-multi-transclude/commit/14461fa63e20aedc662143b7e67904d1a549af51 I definitely added that for a reason. I am trying to recall the scenario that I had identified.

zachsnow commented 9 years ago

@rainboxx ok so here is the example: http://plnkr.co/edit/HwBF26cWS1uZwf8P3ZyU?p=preview

Basically, when you finally remove transcludeContainer it triggers $destroy and breaks the bindings in the transcluded content. This happens whether we do it "by hand" (in the event handler that is present in the code already) or not -- presumably because calling remove on the element that resulted from the transclusion naturally destroys the scope that was created for the transclusion. This is why I originally introduced childScope.

The only way to make it work seems to be to not remove transcludeContainer (until the containing elements are removed -- so I don't think there will be a memory leak). However, that leaves a random hidden div at the end of your content.

I'm probably missing something obvious. If you have anything to add I'd appreciate it.

rainboxx commented 9 years ago

Hm, it was very weird to see the HTML being present as written and not interpolated/parsed by AngularJS, especially since we're using this patch in our application successfully. But then I saw that you're using AngularJS version 1.2.23 while we're using 1.3.15. Updating the Plunker to using AngularJS 1.3.15 makes the example work: http://plnkr.co/edit/H29vtU1l4w97xBcskkU4?p=preview.

It seems that there has been a fix in AngularJS' own $transclude function.

zachsnow commented 9 years ago

Ah good eye. Sounds great, I'll merge it in. Also, your thoughts on dropping the $destroy handler now that we're keeping everything attached to the DOM at all times?

rainboxx commented 9 years ago

I just saw that the bower.json specifies "angular": ">=1.2.16" as dependency. Would it make sense to update this to something higher than 1.3.0?

rainboxx commented 9 years ago

Sorry, missed your edit.

When is the $destroy actually called? I can't see it being called since removing the element won't (or should not) issue a $destroy event on a $scope. I added a console.log to the $destroy handler and it never got called using the given example.

zachsnow commented 9 years ago

Ah, ok -- sounds like I was seeing the bug that was fixed between Angular versions. Thanks.

rainboxx commented 9 years ago

Anything still open?

zachsnow commented 9 years ago

I cut a new release with your fix + upgraded Angular. https://github.com/zachsnow/ng-multi-transclude/releases/tag/0.1.5

rainboxx commented 9 years ago

Awesome, thanks! :-)

I'll update our bower.json in the morning to point to the new release.