dynamically loaded child tags don't get into the parents tags object #1174

Closed
MartinMuzatko opened this Issue Aug 29, 2015 · 13 comments

Projects

None yet

8 participants

@MartinMuzatko
Member

See http://plnkr.co/edit/gvpVcQ8a8dfDQc0MBlFt?p=preview

While the dynamically loaded tag gets displayed, it won't be accessible via this.tags.
Are children tags only added to this.tags object (in parent scope) if their HTML is present when the mount event gets called?

@MartinMuzatko MartinMuzatko changed the title from dynamically loaded tags don't get into the children tag list to dynamically loaded child tags don't get into the parents tag list Aug 29, 2015
@MartinMuzatko MartinMuzatko changed the title from dynamically loaded child tags don't get into the parents tag list to dynamically loaded child tags don't get into the parents tags object Aug 29, 2015
@rsbondi
Contributor
rsbondi commented Aug 29, 2015

Looks like parent of news tag is undefined, need a mechanism for preserving on dynamic mount.

In the mean time you could do this

var p = riot.mount(page)[0]
app.tags[page] = p
@GianlucaGuarini
Member

I am not sure supporting this feature makes really sense. It means that any tag mounted should search whether in its parents there is a riot tag instance, and this could slow down a lot the code especially in the loops. I think the @rsbondi solution is a good workaround. But that's just my opinion

@rsbondi
Contributor
rsbondi commented Aug 30, 2015

#1145 may be related where @AndrewSwerlick says that "the riot.mount strategy breaks parent child relationships", not sure if this is what he was referring to.

I think it would be nice to have an elegant, efficient solution, and maybe exposing more of the internals as suggested in the PR would be a reasonable approach. My example above is a shorthand version but there is more that goes on and it would be good to fully evaluate the tags in a common way.

@AndrewSwerlick

@rsbondi That's exactly what I'm referring to. This is a good workaround, but I also think it deserves to be supported in an elegant and efficient way like you suggest. But knowing this workaround exists means that we could make a utility library external to riot that handles that process. My only concern would be is the .tags property considered to be a stable/public part of the api? It's not documented anywhere that I'm aware of. @GianlucaGuarini can you comment?

UPDATE:

As I'm thinking about this, it still doesn't solve the problem the on the child tag I don't think .parent will be defined, but that's slightly less important.

@rsbondi
Contributor
rsbondi commented Aug 30, 2015

p.parent = app, like I said it was a shorthand version, for illustration.

For me the appeal of riot is the simplicity, the limited api and short learning curve. So I think the key here is finding a balanced way that will not add too much to the api if at all, or handle internally, which has its challenges as @GianlucaGuarini mentioned.

Maybe a simple solution would be a tag.mountChild method that can wrap to riot.mount. My only concern is that we don't make a habit of adding to the api unless the benefit drastically outweighs the cost. Maybe a mountChild mixin would be best. It seems like all that is missing is the tags property getting updated and the parent getting set.

@rsbondi
Contributor
rsbondi commented Aug 30, 2015
@MartinMuzatko
Member

Using this.tags[page] = riot.mount(page)[0] works well enough in my case. I like the mixin too (We definitely need a mixin library in the 2.3 docs or examples, or I'll create an external one 😃 )

I'd agree with @rsbondi to avoid additional public methods in riot to solve this problem.

We could perhaps use additional triggers in riot, so users can decide to manually enable that behavior, kind of like an extended update block. As you probably want to avoid to have such a heavy load on every update event. Then again, the mixin solution or getting the element when mounting works for me just as well, so it is up to you guys.

@jayk
jayk commented Sep 1, 2015

I would like a clean way to do this as well. The use case I have in mind is generating a form dynamically from data using set of riot-based form widgets. I understand the mixin idea and I'm using mixins in other places, but I am concerned that the mixin is too tightly bound to how Riot does things right now... which means when riot gets an update it can easily break, as will all the things that use the mixin. :-(

What we are really talking about is the ability to add a riot-tag after the parent has been mounted, and that seems pretty within the Riot core wheelhouse to me.

From what I can tell, this is really just exposing an official way to trigger 'initChildTag' from riot/lib/browser/tag/util.js - am I missing something there?

Again, since tag handling is pretty much what Riot does, it seems to be adding some simple mechanism to ensure tags can be added properly, even after initial mount, is well within scope. I'm happy to take a swing at a patch, if it is likely to be incorporated.

Edit: I'm totally fine with making it a manual thing (thus avoiding the performance hit on update) I'd just like an official way to do it without creating a hard-binding from my tags to the implementation details of Riot.

@rsbondi rsbondi self-assigned this Dec 19, 2015
@rsbondi
Contributor
rsbondi commented Dec 24, 2015

This really is not a bug, more of the design of how riot.mount works, riot is mounting, not the tag. I think the solution is implementing riot-tag={ expression }, right now riot-tag is not dynamic and only evaluated on riot.mount. I have a WIP version(sorry for the complex example) that I need to clean up and hopefully submit by year end. I think this is a good solution, it is not really adding to the API, just combining existing API features.

@aMarCruz
Member

@rsbondi , I like the mountChild solution, it is simple. riot-tag with expressions looks ugly and guess will generate issues because the life cycle of the context. I'm thinking in the expression returning undefined.

@crisward
Member

I realise there is a fix in the pipeline for the dynamic riot-tag, and the if statement issue. But in the meantime I've release this very small tag which can be used to fix both issues. I had 5 subtags with if statements on, and performance became a real issue, subtag is much faster.

https://github.com/crisward/riot-subtag

npm install riot-subtag
@kokujin
kokujin commented Mar 30, 2016

Any news on dynamically added sub-tags?

@GianlucaGuarini GianlucaGuarini added this to the 3.0.0 milestone Mar 30, 2016
@rsbondi rsbondi referenced this issue Jul 20, 2016
Closed

Events not triggering in child-tag. #1896

1 of 7 tasks complete
@rsbondi rsbondi added the fixed label Jul 24, 2016
@crisward
Member
crisward commented Nov 22, 2016 edited

looks like riot-subtag is now redundant! - will leave live for riot.2.* users. Updated Readme.

@rsbondi rsbondi referenced this issue Dec 9, 2016
Closed

Child tag not updated when parent did update. #2151

0 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment