Attributes with expressions being removed

[fabien]

1. Describe your issue:

I've upgraded from 3.4.1 to 3.7.2 and starting from 3.4.2 I see some attributes being removed although they have a non-empty (string) value. I've narrowed the issue down to this line:

riot/lib/browser/tag/update.js

Line 160 in 9115bf8

if (expr.attr && (!expr.isAttrRemoved || !hasValue || value === false)) {

Which, in my opinion should be as follows:

if (expr.attr && !expr.isAttrRemoved && (!hasValue || value === false)) {

From my testing, !expr.isAttrRemoved will always be true initially, and therefor, the attribute will always be removed but never restored once it does get set eventually. I don't really see why you'd want to set the isAttrRemoved flag, as it seems to imply that attribute won't ever be set again, despite the possibility value gaining a valid value later.

2. Can you reproduce the issue?

http://plnkr.co/edit/tFaKcHP449Usn9KwEFsT?p=preview

See the setTimeout - the attribute is removed initially (by lack of a value) but then never set again.

3. On which browser/OS does the issue appear?

Probably any.

4. Which version of Riot does it affect?

3.4.2 onwards, currently 3.7.2

5. How would you tag this issue?

• Question
• Bug
• Discussion
• Feature request
• Tip
• Enhancement
• Performance

[GianlucaGuarini]

In your demo you forgot to trigger the tag update http://plnkr.co/edit/W0KOUKUwtDWyVf2WlJky?p=preview . I probably the bug is related to something else. Please provide a valid example

[fabien]

Indeed, I forgot! Sorry ... could you please explain the logic behind setting isAttrRemoved?

[GianlucaGuarini]

@fabien the isAttrRemoved is needed mainly to clean up always the riot tags removing unnecessary attributes (options attributes for example). We use a defensive strategy here always remove all the attributes by default on the first render.
Imagine the following case:

<my-el>
   <my-child message={opts.something.myVal} whatever={opts.bla.bla} />
</my-el>

Using isAttrRemoved we can always remove the message and whatever attributes keeping our tags clean unless a user needs explicitly primitive values in the attributes.

This strategy hasn't really changed since riot@2.0.2 it was just a bit optimized for performance reason but I will change it in riot@4.

However I have noticed that we can rely on wasParsedOnce instead of isAttrRemoved so I have updated the riot code using one flag less

[fabien]

@GianlucaGuarini thanks for your explanation. It's really difficult for me to trace down the issue though, despite it being so prevalent in my app. Version 3.7.3 gives me the same trouble, and I don't know why these attributes are not being added again.

Unless of course, they are removed on the first render, and they are not rendered (updated) again. How can you ensure they have their initial value set on the first render that way?

[fabien]

OK, it appears there's a conflict with the particular UI library I'm using: UiKit. It uses the MutationObserver API to act upon tag attributes:

http://plnkr.co/edit/stL0whggah47CB9SsSOS?p=preview

The uk-grid attribute creates a columnar layout: you should see 2 columns, and indeed, the attribute has been added eventually. But apparently, the MutationObserver expects the attribute to be there once the tag has been created. When it's added later, it will not kick in, unless I force it to. There are actually many of these 'directives' in UiKit, and it would be quite a task to identify them all, and then re-trigger it. It would also defeat the purpose of the observer.

@GianlucaGuarini is there any way to make sure these attributes are also set initially?

[GianlucaGuarini]

I would just initialize the UIkit component when you need it http://plnkr.co/edit/2JeMQzByWQFDj0tJyaH2?p=preview Your initial attribute is undefined on the first mount and riot removes it properly. I don't see any bug here

[fabien]

@GianlucaGuarini I'm sorry for all the confusing things, and thank you for your help (and patience).

Turns out there's something different at play altogether - I've updated the example:

http://plnkr.co/edit/stL0whggah47CB9SsSOS?p=preview

It boils down to attributes set on the tag root element itself. These are not set correctly, compared to those from nested elements. Unless I'm missing something (again), it does appear like a bug.

[GianlucaGuarini]

The UIkit apparently applies the grid on the first element it finds. This is unfortunately not a riot issue it's a UIkit one. Automagically initializing components it's not a good idea in this case IMO. Please use the my solution provided here instead

[fabien]

Actually, the second grid isn't initialized because there's an additional <div> that interferes. While I agree that automagical initialization is not ideal, it does work fine (and has been before):

http://plnkr.co/edit/z6lo3ZnKLRNpKEmcMKYl?p=preview

I think it's still something in riot, in the sense that attributes are handled differently, when they are on the root (in a tag definition). I seem to recall similar issues in the past, which had been fixed in the meantime.

[fabien]

@GianlucaGuarini have a look at this: http://plnkr.co/edit/2VLrCGPLp6fEanxoeYX2

It seems that attributes without a value, without any expressions even, are removed by Riot. I think the internal logic should only remove attributes if they are dynamic (and have an expression). In this case, even 'static' attributes are being stripped.

Note that this only happens on the root of tag definitions, not when the tag is mounted with these attributes set on it (as your example above).

[hudelgado]

@GianlucaGuarini do you think i can help with this one also?
Do you have some previous work i can finish or can you give me some hints of where i should look for it?

Thanks

[GianlucaGuarini]

@hudelgado I didn't start with it yet. I need to check how to handle it but I think the components will receive these attributes. The question is in which scope should be they evaluated.. the parent scope or the tag scope?

[GianlucaGuarini]

I am closing this issue because it's a duplicate of #2761 I will track the progress on it on the new issue.