New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Attributes not updating in v.3.4.4 #2343

Closed
fabien opened this Issue May 10, 2017 · 9 comments

Comments

Projects
None yet
3 participants
@fabien
Contributor

fabien commented May 10, 2017

I was seeing various issues that were initially hard to pinpoint. Since I still have yet to create an isolated plunkr, derived from a fairly complex setup, I will try to update this issue later.

I was able to make things work with the following changes in place, exactly like shown here:

https://gist.github.com/fabien/15465c84d75de9aca69cc04c99f48667

What I was seeing:

  • custom tags with attributes like disabled never had their attribute removed when falsy, and never updated after an initial update, causing the disabled state to be stuck irreversibly. It only appears to happen with so-called boolean attributes.

The // remove original attribute part was affecting this behavior (the former, new implementation did not work, the latter - older - does).

  • tags not updating, seeing raw template strings, like {value} instead of the evaluated expression, or evaluated strings not being passed through the whole update cycle.

The omission of setAttr(this.dom, this.attr, this.value); is related to this issue. See below, this is actually a side-effect of #2338

  1. Can you reproduce the issue?

http://plnkr.co/edit/c4i4WiyLjHeXg8E96ppA?p=preview (v.3.4.4 - fails)

http://plnkr.co/edit/6TsSVHam1tgLsPwjJn73?p=preview (v.3.4.3 - works)

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

Chrome Mac, latest

  1. Which version of Riot does it affect?

v.3.4.4

  • reverting to v.3.4.1 worked for both issues mentioned
  • reverting to v.3.4.3 worked for the attributes not updating
  1. How would you tag this issue?
  • Bug
@fabien

This comment has been minimized.

Show comment
Hide comment
@fabien

fabien May 10, 2017

Contributor

With the above code in place, the following test fails:

  • Riot core expressions object attributes get removed once used FAILED

Which seems fairly descriptive of the behavior I'm seeing (but was not expecting, since it has been working fine before).

Contributor

fabien commented May 10, 2017

With the above code in place, the following test fails:

  • Riot core expressions object attributes get removed once used FAILED

Which seems fairly descriptive of the behavior I'm seeing (but was not expecting, since it has been working fine before).

@fabien

This comment has been minimized.

Show comment
Hide comment
@fabien

fabien May 10, 2017

Contributor

This is issue has similarities with #2341 as it happens both within a yielded block as well as in a normal setup. What's similar is that only the initial update is handled correctly.

Contributor

fabien commented May 10, 2017

This is issue has similarities with #2341 as it happens both within a yielded block as well as in a normal setup. What's similar is that only the initial update is handled correctly.

@fabien

This comment has been minimized.

Show comment
Hide comment
@fabien

fabien May 10, 2017

Contributor

@GianlucaGuarini I will try to add a plunkr tomorrow. Meanwhile, if you have any pointers related to those recent changes (their intention and reasoning behind them), that would be really helpful.

Contributor

fabien commented May 10, 2017

@GianlucaGuarini I will try to add a plunkr tomorrow. Meanwhile, if you have any pointers related to those recent changes (their intention and reasoning behind them), that would be really helpful.

@fabien

This comment has been minimized.

Show comment
Hide comment
@fabien

fabien May 11, 2017

Contributor

@GianlucaGuarini here's a plunkr that clearly shows attributes not being updated, they are only set initially (note how they remain red, starting out from black):

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

Contributor

fabien commented May 11, 2017

@GianlucaGuarini here's a plunkr that clearly shows attributes not being updated, they are only set initially (note how they remain red, starting out from black):

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

@fabien

This comment has been minimized.

Show comment
Hide comment
@fabien

fabien May 11, 2017

Contributor

What's interesting is that both changes were introduced following an issue I raised initially.

Like I said before, I respect all the work you put in, and still do.

However, given this, I'm keen to tread carefully in the future. There's a certain frailty that starts to worry me a bit, especially after having invested heavily in a Riot-based setup for my work. I will not discard Riot easily, I really enjoy using it. I'm just a bit careful now.

Also, I'd appreciate it if the invalid label would not be thrown at issues so readily ...

Contributor

fabien commented May 11, 2017

What's interesting is that both changes were introduced following an issue I raised initially.

Like I said before, I respect all the work you put in, and still do.

However, given this, I'm keen to tread carefully in the future. There's a certain frailty that starts to worry me a bit, especially after having invested heavily in a Riot-based setup for my work. I will not discard Riot easily, I really enjoy using it. I'm just a bit careful now.

Also, I'd appreciate it if the invalid label would not be thrown at issues so readily ...

@fabien

This comment has been minimized.

Show comment
Hide comment
@fabien

fabien May 11, 2017

Contributor

Another update, as I discovered this part of the issue is caused by a side-effect:

  • tags not updating, seeing raw template strings, like {value} instead of the evaluated expression, or evaluated strings not being passed through the whole update cycle.

I was able to trace this back to the data-ref attribute being removed, which @papas-source already pointed out, is not a valid solution afterall: #2338 (comment) - it strikes me as an odd way to resolve the 'clobbering' issue I raised as well. Why not re-add the attribute or make sure that arrayishAdd doesn't have such side-effects? In any case data-ref should be left untouched.

A simulated setup shows what happened (using a selector with multiple fallbacks in jQuery, which is used from a mixin that cannot rely solely on refs.elem to be set):

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

The other issue I raised, with (boolean) attributes not updating remains valid, and appears to be pretty serious.

Contributor

fabien commented May 11, 2017

Another update, as I discovered this part of the issue is caused by a side-effect:

  • tags not updating, seeing raw template strings, like {value} instead of the evaluated expression, or evaluated strings not being passed through the whole update cycle.

I was able to trace this back to the data-ref attribute being removed, which @papas-source already pointed out, is not a valid solution afterall: #2338 (comment) - it strikes me as an odd way to resolve the 'clobbering' issue I raised as well. Why not re-add the attribute or make sure that arrayishAdd doesn't have such side-effects? In any case data-ref should be left untouched.

A simulated setup shows what happened (using a selector with multiple fallbacks in jQuery, which is used from a mixin that cannot rely solely on refs.elem to be set):

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

The other issue I raised, with (boolean) attributes not updating remains valid, and appears to be pretty serious.

@fabien fabien changed the title from Various issues after upgrading from v.3.3.2 to Attributes not updating in v.3.4.3 May 11, 2017

@fabien fabien changed the title from Attributes not updating in v.3.4.3 to Attributes not updating in v.3.4.4 May 11, 2017

@ramonakira

This comment has been minimized.

Show comment
Hide comment
@ramonakira

ramonakira May 12, 2017

Hi fabien,

The issue with your first plunkr might be just an html5 issue. The disabled attribute is, according to mdn, only supported on button, command, fieldset, input, keygen, optgroup, option, select and textarea elements, but not h2.

I've changed your plunkr to be html-compliant and it seems to just work as expected to me?
http://plnkr.co/edit/fB7zcCVqxObLLfmVBv8U?p=preview

It's still html we're dealing with here, can't just go around doing stuff just because we want it to work. :)

ramonakira commented May 12, 2017

Hi fabien,

The issue with your first plunkr might be just an html5 issue. The disabled attribute is, according to mdn, only supported on button, command, fieldset, input, keygen, optgroup, option, select and textarea elements, but not h2.

I've changed your plunkr to be html-compliant and it seems to just work as expected to me?
http://plnkr.co/edit/fB7zcCVqxObLLfmVBv8U?p=preview

It's still html we're dealing with here, can't just go around doing stuff just because we want it to work. :)

@GianlucaGuarini GianlucaGuarini added bug and removed to verify labels May 12, 2017

@fabien

This comment has been minimized.

Show comment
Hide comment
@fabien

fabien May 12, 2017

Contributor

@ramonakira thanks for your reply. I thought about the overlap with the html5 attribute disabled at first, too. And while it does work in the updated plunkr, it does not explain why the test attribute isn't updated either (see: http://plnkr.co/edit/039YbpFxtPSq4HdSCrTZ?p=preview).

You changed from disabled to using class which is already treated in a special way inside of Riot.

Moreover, as you can see from the v.3.4.3 version, it simply worked as expected before.

I suspect that Riot handles native tags differently from custom ones, which might explain the difference in behavior.

Contributor

fabien commented May 12, 2017

@ramonakira thanks for your reply. I thought about the overlap with the html5 attribute disabled at first, too. And while it does work in the updated plunkr, it does not explain why the test attribute isn't updated either (see: http://plnkr.co/edit/039YbpFxtPSq4HdSCrTZ?p=preview).

You changed from disabled to using class which is already treated in a special way inside of Riot.

Moreover, as you can see from the v.3.4.3 version, it simply worked as expected before.

I suspect that Riot handles native tags differently from custom ones, which might explain the difference in behavior.

@ramonakira

This comment has been minimized.

Show comment
Hide comment
@ramonakira

ramonakira May 12, 2017

Indeed, you are right.

Indeed, you are right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment