Skip to content

Improving the scope attributes #570

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

Closed
Zirro opened this issue May 6, 2017 · 21 comments
Closed

Improving the scope attributes #570

Zirro opened this issue May 6, 2017 · 21 comments

Comments

@Zirro
Copy link
Contributor

Zirro commented May 6, 2017

I appreciate the attention that's gone into making Svelte output readable code. It makes it easier to debug and understand what is happening at a glance. However, one part which I feel stands in contrast to this is the svelte-[hash] attributes. They're pretty verbose, making the rendered DOM harder to inspect and consume visually, while offering no human-friendly clue as to where they belong.

Under the assumption that the purpose of the hash is only to serve as a unique identifier for a component, I believe these concerns could be resolved by replacing the hash with the name of a component instead. In the event of two components having the same name, they can be deconflicted by appending a number as is done in the script.

Having made a few modifications to the compiler based on this idea, I compiled the templates for svelte.technology. Here's a compact example showing before and after:

<div class="secondary" svelte-3279509091>
  <ul class="guide-toc" svelte-2365569553>
    <li svelte-2365569553><a class="section " href="/guide#introduction" svelte-2365569553>Introduction</a></li>
  </ul>
</div>

https://gist.github.com/Zirro/79e016d2e84ca98976c2fa5a5ab7b9b9

<div class="secondary svelte•nav">
  <ul class="guide-toc svelte•guidecontents">
    <li class="svelte•guidecontents"><a class="section svelte•guidecontents" href="/guide#introduction">Introduction</a></li>
  </ul>
</div>

https://gist.github.com/Zirro/708b96d3046b72af98971477fa9282ea

You'll note that there's another change as well - it's using classes instead. I decided to try this for two reasons - one of fairly little importance, which is that Firefox's view-source renders attributes in bold black letters, making them quite a distraction.

More importantly is that selecting elements by attributes is measurably slower than doing so with classes. I figured that a class can do the job just as well, provided that it uses a name with virtually no risk of clashing with ones that site developers would add.

For this, I conveniently came across • in the <title> of the Svelte homepage. I believe it strikes a good balance, showing that the class is special without being a distraction. It can also be set together with other classes in a single .className instead of a separate setAttribute().

I suppose this is two suggestions in one. The hash -> name and attribute -> class changes can both be discussed separately.

What do you think? Is there something which I've overlooked that would make this impractical?

@Rich-Harris
Copy link
Member

Thanks — this is useful feedback, I agree about the verbosity. It is a bit ugly.

The main reason for using hashes is that it guarantees consistency — if you render a component on the server and again in the client, the hashes will always match as long as it's the same source. This means the styles are always correct and never out of date.

We could use names instead if we were prepared to loosen that guarantee, but conflicts would be a problem — the compiler only considers components in isolation, it doesn't know that you have two <Widget> components that are different. So the compiler itself couldn't deconflict, it would have to be the responsibility of e.g. a bundler plugin, which has a birds eye view of everything and can feed information into the compiler.

Using class names instead of attributes could certainly work, even if classes are dynamic. It would make the markup slightly more verbose...

<!-- this... -->
<div class='svelte-123456'></div>

<!-- instead of this -->
<div svelte-123456></div>

...but if there's a performance gain to be had then it's probably worth it. Do you have any numbers you can share?

@Zirro
Copy link
Contributor Author

Zirro commented May 6, 2017

...the compiler only considers components in isolation, it doesn't know that you have two components that are different

Ah, I see. I found $2 appended to my duplicate component in the bundle, but I realise now that this was probably courtesy of Rollup and not the Svelte compiler by itself.

...but if there's a performance gain to be had then it's probably worth it. Do you have any numbers you can share?

I'm not sure if using querySelectorAll() on jsperf accurately reflects the way browsers work while styling elements, but I put together this test: https://jsperf.com/queryselectorall-attr-vs-class-selector

When I try it with Firefox and Safari I'm actually not seeing much of a difference between attributes and classes. Chrome does show a more significant advantage when selecting only .class instead of [attr], but that along with the results of the other browsers is still not nearly as much as I had expected.

I suppose browsers today have been optimised to the point where it might only come into play when there are thousands or even tens of thousands of elements involved. http://stevesouders.com/efws/css-selectors/csscreate.php which I found through a comment of yours (which also mentions the performance concerns) can be used to test those scenarios, and do show a greater difference when I test, but I'm not sure if that's reason enough for this change.

@Glenf
Copy link

Glenf commented May 8, 2017

The selector speed issue was relevant about 10 years ago in IE6 and IE7 era browsers. I think currently the selector speed is a non-issue. If the page/app is slow it's because of other issues. There might be some issues in slower and cheaper smart phones but I've not seen these kinds of problems in years. I think the attribute as a selector is considered as a bad practice.

I'd like to see the attributes moved to class names.

@Rich-Harris
Copy link
Member

@Glenf if not for performance reasons, why are attribute selectors bad practice?

@Glenf
Copy link

Glenf commented May 8, 2017

@Rich-Harris Actually I think I'm mixing things up here. Using attributes as selectors in CSS is something that's not a good practice. Partly because of css specificity and partly because of the selector speed.

In this case since attribute selector is something that actually specifies the style even more the first part is kind of irrelevant. The second part, I'm not sure if speed is an issue.

@Zirro
Copy link
Contributor Author

Zirro commented May 8, 2017

The speed of various selectors were discussed more recently than the times of IE6/7 for sure - I recall seeing articles between 2011-2014 - but having looked further into it now it seems safe to say that the performance difference between the selector types are trivial in the browsers of today.

My main interest with this issue was to see whether the hashes could be turned into something more useful/less intrusive to human eyes. Since that may not be possible without adverse consequences, it's okay for me if we close this issue, though I'll let you decide if there is something left to discuss.

@jslegers
Copy link

jslegers commented Aug 7, 2017

Using attributes as selectors in CSS is something that's not a good practice. Partly because of css specificity

Specificity isn't really an issue, considering attribute selectors and class selectors have the same specificity. That means you can replace an attribute selector by a class selector or vice versa without impacting specificity.

More importantly is that selecting elements by attributes is measurably slower than doing so with classes.

The notion that attribute based selectors are slower than class based selectors used to be wide-spread among CSS experts, but I haven't seen much data actually supporting it besides Steve Souders work on CSS selectors from 2009. A lot has changed since then, and today it seems we can pretty much ignore performance differences between selectors.

having looked further into it now it seems safe to say that the performance difference between the selector types are trivial in the browsers of today.

The main reason I avoided the use of attribute based selectors because they weren't supporter in IE < 7, Opera < 9 and Safari < 3, whereas class selectors had pretty much universal support. However, as those browsers aren't relevant anymore, there really is no good reason anymore to avoid attribute based selectors IMO.

So I'm OK with using svelte-xxxxxxxxxx attributes - instead of classes - for scoping. Still, you could always add a boolean flag in the configuration that allows people to choose whether they want class selectors or attribute selectors, similar to the cascade flag (#607). It seems a rather simple improvement to me for those who insist on using class selectors.

I do have an issue with use of randomized hash values, though. Besides being rather ugly and non-semantic, these attributes also prevent efficient component-specific global theming. See #379 (comment) for details.

The main reason for using hashes is that it guarantees consistency — if you render a component on the server and again in the client, the hashes will always match as long as it's the same source. This means the styles are always correct and never out of date.

If you add a check to validate whether the id / name is unique (and raise an error if it's not), shouldn't that guarantee the same consistency as using auto-generated tags?

Anyway, each time you re-build your code, the value of the hash changes, which means that you can't create a global theme to override the component-specific styles of a particular component without impacting other components.

This would be fixed if we could just set an (optional) id or name attribute for our components and use that for scoping instead of a generated hash when this attribute has been set (as @Zirro suggested).

For me, adding this feature would be a major improvement!

This was referenced Aug 7, 2017
@PaulBGD
Copy link
Member

PaulBGD commented Aug 7, 2017

I do have an issue with use of randomized hash values

The point is that they're not randomized, they're a hash. They guarantee that whenever you compile your code the hash is the exact same.

This would be fixed if we could just set an (optional) id or name attribute for our components

I think your issue with global theming comes down to this, but I feel like this solution already works:

<div class="my-component"></div>

In response to your comment on #379, it seems in that case you would simply go the route of not including "default" styles in your components. If you want to style them with a global theme, then use a global theme.

@jslegers
Copy link

jslegers commented Aug 8, 2017

The point is that they're not randomized, they're a hash. They guarantee that whenever you compile your code the hash is the exact same.

What if I modify my code? I mean... I don't want to generate my CSS themes every time I change a line in my components...

How exactly is the hash generated?

I think your issue with global theming comes down to this, but I feel like this solution already works:

<div class="my-component"></div>

In reality, my component would look something like this :

<div svelte-1535408840 class="my-component">
  <div class="button">Press me</div>
</div>
/* generated component CSS */
[svelte-1535408840] .button {
  background : #ccc;
}

/* hand-written global theme CSS */
.my-component .button {
  background : #ccc;
}

This bothers me for several reasons :

  • The svelte-1535408840 is plain ugly and non-semantic, and not very human-friendly
  • Having a svelte-1535408840 attribute as well as an my-component class in my HTML is redundant
  • Having a svelte-1535408840 in my generated CSS and a my-component class in my global theme makes it harder to both read and debug my CSS

It would be infinitely better to be able to do something like this :

<div svelte-component>
  <div class="button">Press me</div>
</div>
/* generated component CSS */
.svelte-component .button {
  background : #ccc;
}

/* hand-written global theme CSS */
.svelte-component .button {
  background : #ccc;
}

In response to your comment on #379, it seems in that case you would simply go the route of not including "default" styles in your components. If you want to style them with a global theme, then use a global theme.

The use case I have in mind for Svelte is a UI component framework built on top of a CSS framework, that itself can be used as a generic foundation for any JS project. Without being able to use three layers of CSS - as described in #379 - this simply won't work.

@Rich-Harris
Copy link
Member

Another data point, suggesting that appending classes may be better than adding an attribute:

@Conduitry
Copy link
Member

I think we can close this. The 'random' codes are needed for encapsulation of styles - and we have another issue (#1118) dedicated to the task of switching from attributes to classes. If someone disagrees, this can be reopened.

@mrkishi
Copy link
Member

mrkishi commented Feb 7, 2018

I think we could still improve the visual noise of the hash, even if switching to classes.

Currently:

With classes:

With base26 instead of decimal:

With component name instead of svelte:

@Conduitry
Copy link
Member

Using base26 and the component name are both interesting ideas! Reopening.

@Conduitry Conduitry reopened this Feb 7, 2018
@Rich-Harris
Copy link
Member

See #1192 — uses classes, base 36, and (possibly, not sure yet if we want this) foo-xyz instead of svelte-xyz.

@Rich-Harris
Copy link
Member

Just released 1.57 which switches to classes and base36. I decided against defaulting to using the component name, since it reduces compressability, but I think it would be worth making it configurable. From #1192 (comment):

const counts = {};
const incr = id => {
  if (!counts[id]) counts[id] = 0;
  return counts[id] += 1;
};

const { code, css, map } = svelte.compile(source, {
  class: ({ name = 'svelte', filename, hash }) => `${name}-${incr(name)}`
});

Will leave this open until we get round to implementing that.

@arxpoetica
Copy link
Member

arxpoetica commented Mar 18, 2018

Somehow I missed this issue/thread. I'll just chime in on one thing which has to do with semantics. (It's already been said, but I think it bears repeating, perhaps ad nauseam 😅.) I thought one of the points of semantics on the web was readability; base36 hashes not being such? Maybe a fools errand, but this might also go to a component marketplace. Dropping something in place and having a never-changing semantic namespace that someone knows by name.

FYI, hashes might still be the right thing, but this is definitely a problem that needs to be thought out within the larger context of dropping components into random and sundry places.

@Rich-Harris
Copy link
Member

@arxpoetica Your existing semantic classes etc are preserved for that reason. The additional scoping class has one job - avoiding clashes - and so it kind of has to be based on a hash, doubly so if we're talking about a component marketplace

@arxpoetica
Copy link
Member

Huh. No need to explain; I won't beat a dead horse and I totally trust your mentality on this, but I can't put two and two together. 😄

@arxpoetica
Copy link
Member

p.s. I've come around to Rich's thinking on this...

@zeng450026937

This comment has been minimized.

@kaisermann
Copy link
Member

The cssHash({ hash, css, name, filename }) compiler option was released in v3.34.0 🎉 . This can be closed now.

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

No branches or pull requests