Skip to content

use base 36 for style classes #1192

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

Merged
merged 3 commits into from
Mar 8, 2018
Merged

use base 36 for style classes #1192

merged 3 commits into from
Mar 8, 2018

Conversation

Rich-Harris
Copy link
Member

This replaces svelte-123456 with foo-2n9c, where foo is the name of the component.

A couple of thoughts:

  • Maybe we'd prefer base 26 (i.e. just a-z)? @kristoferbaxter also suggested using base 16 (for reasons relating to compressability that I don't fully understand!)
  • using foo- instead of svelte- is useful in that you can more easily see which component an element/style belongs to, but it's presumably less compressable. (Also, probably not important but I've heard of people discovering Svelte because they say svelte-123456 attributes in someone's markup...)

@codecov-io
Copy link

codecov-io commented Feb 25, 2018

Codecov Report

Merging #1192 into gh-1118 will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           gh-1118    #1192   +/-   ##
========================================
  Coverage    91.67%   91.67%           
========================================
  Files          126      126           
  Lines         4575     4575           
  Branches      1501     1501           
========================================
  Hits          4194     4194           
  Misses         158      158           
  Partials       223      223
Impacted Files Coverage Δ
src/utils/hash.ts 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f01115...3f7f237. Read the comment docs.

@Conduitry
Copy link
Member

Is options.name ever falsy? Does that default to 'SvelteComponent'? I did see that in a lot (all?) of the tests, the names are sveltecomponent-*, which doesn't seem particularly useful, and is extra characters for nothing. I'm ambivalent about using classes named after the component, but I do think that using sveltecomponent if no options.name is passed is not really desirable.

@Rich-Harris
Copy link
Member Author

It can be falsy if you're using the compiler directly. It defaults to 'svelte'...

this.id = `${name ? name.toLowerCase() : 'svelte'}-${hash(parsed.css.content.styles)}`;

...but in the tests, SvelteComponent is specified, hence sveltecomponent-xyz. I share your ambivalence though.

@Conduitry
Copy link
Member

Oh ok cool. Yeah I saw that line in the PR, but I wasn't actually sure that it did anything, because I had forgotten the SvelteComponent name was from the tests, and was not a default that came from the code itself. But yeah I feel like I'd want other people to weigh in on whether to use the component name in the class names at all.

@dschulten
Copy link

when looking into style issues with numerous nested components and cascade:false I found it difficult to understand where the styles came from. Therefore +1

@kristoferbaxter
Copy link

It's an interesting tradeoff removing svelte- and moving to per component names. A lot more discoverable for development (and scraping) purposes.

@kristoferbaxter also suggested using base 16 (for reasons relating to compressability that I don't fully understand!)

I was recommending something greater than base 9 and all of these meet the mark!

However, you might also consider a counter instead of a hash given the additional scoping from the new component name prefix. My hunch is this would reduce the size of style transmission without losing any additional readability.

@Rich-Harris
Copy link
Member Author

However, you might also consider a counter instead of a hash given the additional scoping from the new component name prefix. My hunch is this would reduce the size of style transmission without losing any additional readability.

I was thinking that as a follow-up to this PR, maybe it could be entirely user-configurable:

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)}`
});

That way we'd still get the correctness-guaranteeing behaviour (${name}-${hash}) out of the box, but it could be set up to use incrementing in situations where it makes sense to do so (e.g. there's no SSR, or the process is controlled a la Sapper), or if people were really byte-conscious they could do something like -${hash}. Perhaps that's too much boilerplate for common cases though.

Separately, it occurred to me that there's another reason not to use options.name — it reduces opportunities for deduplication, if you happened to have two components with identical styles but different names (a CSS minifier could remove one of them). That might be too much of an edge case to bother with though.

@kristoferbaxter
Copy link

I was thinking that as a follow-up to this PR, maybe it could be entirely user-configurable

Agree completely. Nice followup and sets a good precedence for overridable defaults.

Separately, it occurred to me that there's another reason not to use options.name — it reduces opportunities for deduplication, if you happened to have two components with identical styles but different names (a CSS minifier could remove one of them). That might be too much of an edge case to bother with though.

I see this as another reason to support overridable defaults for this case. Most implementations would be well suited by ${name}-${hash} but there might be cases where an author (or a future optimisation) could determine using just ${count} works.

Reusable styles (ie CSS Blocks or Atomic CSS) would be a nice future step for something like Sapper.

@m-sio
Copy link

m-sio commented Dec 9, 2019

However, you might also consider a counter instead of a hash given the additional scoping from the new component name prefix. My hunch is this would reduce the size of style transmission without losing any additional readability.

I was thinking that as a follow-up to this PR, maybe it could be entirely user-configurable:

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)}`
});

That way we'd still get the correctness-guaranteeing behaviour (${name}-${hash}) out of the box, but it could be set up to use incrementing in situations where it makes sense to do so (e.g. there's no SSR, or the process is controlled a la Sapper), or if people were really byte-conscious they could do something like -${hash}. Perhaps that's too much boilerplate for common cases though.

Separately, it occurred to me that there's another reason not to use options.name — it reduces opportunities for deduplication, if you happened to have two components with identical styles but different names (a CSS minifier could remove one of them). That might be too much of an edge case to bother with though.

Sorry if I am missing something, but where do I put this code in order for it to work?

@Conduitry
Copy link
Member

Nowhere, it's not supported yet - it was just an idea for a future feature.

7nik pushed a commit to 7nik/svelte that referenced this pull request Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants