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

CSS-Prefixing breaks Virtual + Data-is #2511

Closed
arendtio opened this Issue Dec 12, 2017 · 16 comments

Comments

Projects
None yet
5 participants
@arendtio

arendtio commented Dec 12, 2017

  1. Describe your issue: When you use a tag as a virtual in combination with data-is the tag's CSS classes are not being applied to it. For me it looks as if this problem arises from the fact that the CSS is prefixed with the tag name (or the equivalent data-is attribute), but neither exists in the final DOM.

  2. Can you reproduce the issue? Yes. The example shows 3 squares, but the last one has no blue background as the .square class doesn't get applied to it: http://plnkr.co/edit/nkkUlwGxEaUzcvjTn2m5?p=preview

  3. On which browser/OS does the issue appear? Linux x86_64; Chromium 62 & Firefox 57

  4. Which version of Riot does it affect? 3.7.4

  5. How would you tag this issue?

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

This comment has been minimized.

Show comment
Hide comment
@sourcegr

sourcegr Dec 12, 2017

Now that is a tough one :/

sourcegr commented Dec 12, 2017

Now that is a tough one :/

@arendtio

This comment has been minimized.

Show comment
Hide comment
@arendtio

arendtio Dec 12, 2017

Searching for a workaround: Do you know if there is a way to turn off CSS prefixing/scoping for a tag? As far as I remember CSS scoping was a feature which had to be turned on in riot 2.x and became on by default in riot 3.x but is there a kill switch somewhere? I tried scoped="false" but that doesn't seem to work.

arendtio commented Dec 12, 2017

Searching for a workaround: Do you know if there is a way to turn off CSS prefixing/scoping for a tag? As far as I remember CSS scoping was a feature which had to be turned on in riot 2.x and became on by default in riot 3.x but is there a kill switch somewhere? I tried scoped="false" but that doesn't seem to work.

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Dec 13, 2017

Member

the virtual tag was never designed to be used in that way! The virtual tag can help you in case your if or each directives need temporary wrapper tag like for example a <dl> list. If you combine data-is with a virtual tag you are basically creating a riot tag without any wrapper or root component and that shouldn't be allowed. I will try to dispatch an error preventing this error

Member

GianlucaGuarini commented Dec 13, 2017

the virtual tag was never designed to be used in that way! The virtual tag can help you in case your if or each directives need temporary wrapper tag like for example a <dl> list. If you combine data-is with a virtual tag you are basically creating a riot tag without any wrapper or root component and that shouldn't be allowed. I will try to dispatch an error preventing this error

@sourcegr

This comment has been minimized.

Show comment
Hide comment
@sourcegr

sourcegr Dec 13, 2017

You are right....

I believe that a note about this behavior in the virtual section of the manual, is enough.

sourcegr commented Dec 13, 2017

You are right....

I believe that a note about this behavior in the virtual section of the manual, is enough.

@arendtio

This comment has been minimized.

Show comment
Hide comment
@arendtio

arendtio Dec 14, 2017

Interesting, especially because the guide states exactly that use case:

virtual however is not exclusive to looping and can be used in conjuction with if and data-is for any tag

And to be honest, I stumbled upon that use case a couple of times now. To set this in context: I don't like the virtual tag and try to avoid it whenever possible. I use it only when other option would cause even more pain.

For example at the moment I use it as a workaround until #2508 is fixed. So yes, in an ideal world it should not be necessary to use virtual at all. But there are cases where it is useful for loops or conditions and sometimes even for riot-tags. So solving this issue by disallowing the combination of virtual and data-is would be a bad choice in my opinion.

Better options:

  1. Deactivate CSS scoping for virtual tags by default.
  2. Make it possible to deactivate CSS Scoping per tag

@GianlucaGuarini Don't get me wrong. I totally understand your pursuit of a clean lib with a minimal feature set and I love the smart default choices of riot (e.g. CSS scoping). But if you remove too much, there are no means for users to build workarounds when bugs appear or to build around misconceptions (e.g. yield evaluation).

arendtio commented Dec 14, 2017

Interesting, especially because the guide states exactly that use case:

virtual however is not exclusive to looping and can be used in conjuction with if and data-is for any tag

And to be honest, I stumbled upon that use case a couple of times now. To set this in context: I don't like the virtual tag and try to avoid it whenever possible. I use it only when other option would cause even more pain.

For example at the moment I use it as a workaround until #2508 is fixed. So yes, in an ideal world it should not be necessary to use virtual at all. But there are cases where it is useful for loops or conditions and sometimes even for riot-tags. So solving this issue by disallowing the combination of virtual and data-is would be a bad choice in my opinion.

Better options:

  1. Deactivate CSS scoping for virtual tags by default.
  2. Make it possible to deactivate CSS Scoping per tag

@GianlucaGuarini Don't get me wrong. I totally understand your pursuit of a clean lib with a minimal feature set and I love the smart default choices of riot (e.g. CSS scoping). But if you remove too much, there are no means for users to build workarounds when bugs appear or to build around misconceptions (e.g. yield evaluation).

GianlucaGuarini added a commit that referenced this issue Dec 27, 2017

@laszlof

This comment has been minimized.

Show comment
Hide comment
@laszlof

laszlof Dec 28, 2017

Nice that this was fixed, however, this was a breaking change, and should have probably gone into a major release. We noticed this when our travis builds (set to "riot:^3.7.0") started breaking today.

I do agree this was a bug, and virtual should not have been used this way (even if thats what the docs say), but people tend to follow documentation, which stated it was "ok" to use virtual with data-is.

Fortunately our use of this is fairly limited, so it shouldnt take too much time to work around. But in the meantime we'll be locking our version to 3.7.4 until we get it resolved.

laszlof commented Dec 28, 2017

Nice that this was fixed, however, this was a breaking change, and should have probably gone into a major release. We noticed this when our travis builds (set to "riot:^3.7.0") started breaking today.

I do agree this was a bug, and virtual should not have been used this way (even if thats what the docs say), but people tend to follow documentation, which stated it was "ok" to use virtual with data-is.

Fortunately our use of this is fairly limited, so it shouldnt take too much time to work around. But in the meantime we'll be locking our version to 3.7.4 until we get it resolved.

GianlucaGuarini added a commit to riot/riot.github.io that referenced this issue Dec 28, 2017

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Dec 28, 2017

Member

@laszlof thanks for the reminder. Would a console.error be more appropriate for such a case?

Member

GianlucaGuarini commented Dec 28, 2017

@laszlof thanks for the reminder. Would a console.error be more appropriate for such a case?

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Dec 28, 2017

Member

I mean even if we print a console.error your unit test could fail harder due to the side effects of riot tags created without any root component. This could let you waste hours before you will find out the issue.
I think I will keep the exception error sorry

Member

GianlucaGuarini commented Dec 28, 2017

I mean even if we print a console.error your unit test could fail harder due to the side effects of riot tags created without any root component. This could let you waste hours before you will find out the issue.
I think I will keep the exception error sorry

@laszlof

This comment has been minimized.

Show comment
Hide comment
@laszlof

laszlof Dec 28, 2017

@GianlucaGuarini thats fine, and understandable. The only solution would be to back the change out and put it in 4.0 instead. But this was more as a comment for future changes that may be break things.

laszlof commented Dec 28, 2017

@GianlucaGuarini thats fine, and understandable. The only solution would be to back the change out and put it in 4.0 instead. But this was more as a comment for future changes that may be break things.

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Dec 28, 2017

Member

@laszlof alright it's fair enough. Let's go for the console.error for now. Riot@3.8.1 is on its way ;)

Member

GianlucaGuarini commented Dec 28, 2017

@laszlof alright it's fair enough. Let's go for the console.error for now. Riot@3.8.1 is on its way ;)

@adrian-enspired

This comment has been minimized.

Show comment
Hide comment
@adrian-enspired

adrian-enspired Dec 28, 2017

i'm fine with this resolution, but this should be kept in mind for future changes. Minor versions are not for breaking changes. A deprecation warning is more appropriate.


edit

Let's go for the console.warn for now. Riot@3.8.1 is on its way ;)

this sounds good.

adrian-enspired commented Dec 28, 2017

i'm fine with this resolution, but this should be kept in mind for future changes. Minor versions are not for breaking changes. A deprecation warning is more appropriate.


edit

Let's go for the console.warn for now. Riot@3.8.1 is on its way ;)

this sounds good.

GianlucaGuarini added a commit that referenced this issue Dec 28, 2017

@GianlucaGuarini

This comment has been minimized.

Show comment
Hide comment
@GianlucaGuarini

GianlucaGuarini Dec 28, 2017

Member

Ok now in riot@3.8.1 it's just a console.warn so you can keep screwing the <virtual> tag as much as you like 😄

Member

GianlucaGuarini commented Dec 28, 2017

Ok now in riot@3.8.1 it's just a console.warn so you can keep screwing the <virtual> tag as much as you like 😄

@arendtio

This comment has been minimized.

Show comment
Hide comment
@arendtio

arendtio Dec 29, 2017

I am starting to wonder how thing are being solved in this project...

You have three features:
A: virtual tag
B: data-is attribute
C: CSS scoping

So using A&B together is documented and works fine, but makes it hard/impossible to use in combination with C. So you want to disallow the combination of A&B instead of just deactivating C in combination with A&B. Yes, you could say that if A&B are not allowed together you would not have the complication with C, but you are completely missing that there are valid use cases where combining A&B makes sense and by disallowing the A&B combination your are killing those.

Yes, I understand that you are trying to fix the root cause and I don't know where else in riot you rely on the root tag being existent, but there shouldn't be that many places (otherwise this bug would have been found earlier).

But as all others seem to agree, I feel like I should stop bothering you with bugs.

arendtio commented Dec 29, 2017

I am starting to wonder how thing are being solved in this project...

You have three features:
A: virtual tag
B: data-is attribute
C: CSS scoping

So using A&B together is documented and works fine, but makes it hard/impossible to use in combination with C. So you want to disallow the combination of A&B instead of just deactivating C in combination with A&B. Yes, you could say that if A&B are not allowed together you would not have the complication with C, but you are completely missing that there are valid use cases where combining A&B makes sense and by disallowing the A&B combination your are killing those.

Yes, I understand that you are trying to fix the root cause and I don't know where else in riot you rely on the root tag being existent, but there shouldn't be that many places (otherwise this bug would have been found earlier).

But as all others seem to agree, I feel like I should stop bothering you with bugs.

@adrian-enspired

This comment has been minimized.

Show comment
Hide comment
@adrian-enspired

adrian-enspired Feb 9, 2018

@arendtio fwiw, i agree that <virtual data-is="some-tag"/> is conceptually wrong and should be removed. just not in a minor version, because it breaks things on a release which supposedly won't break anything.

adrian-enspired commented Feb 9, 2018

@arendtio fwiw, i agree that <virtual data-is="some-tag"/> is conceptually wrong and should be removed. just not in a minor version, because it breaks things on a release which supposedly won't break anything.

@arendtio

This comment has been minimized.

Show comment
Hide comment
@arendtio

arendtio Feb 9, 2018

@adrian-enspired fwiw, I don't care anymore. I used riot for years (at least since early 2015), told others how great it was and gave feedback on what could be done better, but when I see how decisions are made here, I understand why core issues don't get solved. The only thing I don't understand is how something as great as riot could emerge from such decision making. My only explanation is that the core team must have changed over the years.

Nevertheless, do whatever you want with parent.parent.parent., yield, virtual, data-is I will not have a problem with that anymore. Instead I will worry about how to convert all my riot stuff to Vue or whatever. Have fun.

arendtio commented Feb 9, 2018

@adrian-enspired fwiw, I don't care anymore. I used riot for years (at least since early 2015), told others how great it was and gave feedback on what could be done better, but when I see how decisions are made here, I understand why core issues don't get solved. The only thing I don't understand is how something as great as riot could emerge from such decision making. My only explanation is that the core team must have changed over the years.

Nevertheless, do whatever you want with parent.parent.parent., yield, virtual, data-is I will not have a problem with that anymore. Instead I will worry about how to convert all my riot stuff to Vue or whatever. Have fun.

@sourcegr

This comment has been minimized.

Show comment
Hide comment
@sourcegr

sourcegr Feb 9, 2018

@arendtio same here. Same feelings, same thoughts.

sourcegr commented Feb 9, 2018

@arendtio same here. Same feelings, same thoughts.

@GianlucaGuarini GianlucaGuarini referenced this issue Mar 30, 2018

Closed

`<virtual data is=` not working at all #2564

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