Skip to content
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

Allow to customize the css scope class via compilerOptions.scopeClass method #4377

Closed

Conversation

kaisermann
Copy link
Member

@kaisermann kaisermann commented Feb 6, 2020

Possible implementation for the custom scope class getter discussed in #570.

  • Added a scopeClass property to the compiler options which should receive a method with the following signature: ({ hash:string, filename hash:string }) => string
  • If no scopeClass method is provided, the compiler uses the default method that returns svelte-{hash}

If and after this is merged, the docs should be also updated.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases features are absent for a reason.
  • This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
  • Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Tests

  • Run the tests tests with npm test or yarn test)

@kaisermann kaisermann force-pushed the feat/allow-custom-scope-class branch 3 times, most recently from 0b2735e to 8ebc0fe Compare February 6, 2020 00:30
@kaisermann kaisermann marked this pull request as ready for review February 6, 2020 00:33
@kaisermann kaisermann changed the title Allow to customize the css scope class Allow to customize the css scope class via compilerOptions.scopeClass method Feb 6, 2020
@Conduitry
Copy link
Member

Have you taken a look at the potential API described in Rich's comment? I'm not sure what I'd want this option to be called (I'm not particularly thrilled with either class or scopeClass), but I think passing the component's name in addition to its filename makes sense.

@kaisermann kaisermann force-pushed the feat/allow-custom-scope-class branch 2 times, most recently from 1facc52 to d68f723 Compare February 9, 2020 16:34
@kaisermann
Copy link
Member Author

kaisermann commented Feb 9, 2020

I did look at it, but when testing it was always returning undefined. However I just discovered I was using the compile_options.name directly, instead of the name prop passed to the Component.ts constructor 😳Done, added it!

I'm also not the biggest fan of scopeClass and class is too generic, it doesn't say what the prop is supposed to do. Naming this one is kind of hard, but I'll throw some names here:

  • scopeClass
  • class
  • classGenerator
  • cssHash
  • cssScope
  • cssId (🤔 hmmm)

@dreitzner
Copy link
Contributor

dreitzner commented May 2, 2020

  1. Maybe we could call it stylesheet options and have an option inside of it for classnames. This would allow for better readability in the config I think as the options would be "scoped"

  2. We should definitely add safety checks for the classes (can implement some from my PR)

  3. One thing I stumbled upon in my research, is that the allowed characters for class names could be expended by escaping certain characters (: => \:). We might want to consider this too as it enables even smaller class names in theory.

  4. How would we test, or should we test the validity of css selectors? (See examples from my PR)

@clintwood
Copy link

Is this something that will happen? It's a bit concerning that there are so many outstanding PR's!

@clintwood
Copy link

@kaisermann @Conduitry is there any chance of getting this merged - are we just stuck on the naming of the option?

I'd suggest cssScope or classScope.

Thanks!

@neilpanchal
Copy link

At the very least, allow the ability to change the hard coded prefixsvelte- to whatever brandname- so that if someone inspects the source, it is atleast a bit more "professional" for the lack of a better word. Exposing what framework was used to generate a website isn't ideal (although good for the publicity of the framework).

@dominikg
Copy link
Member

dominikg commented Feb 9, 2021

Just ran into this today and being able to set a stable scope class during compile would be helpful for vite-plugin-svelte.
What's missing for this to land?

@dominikg
Copy link
Member

I followed some of the links in this discussion and it seems that what's left to do here is update the PR to current and find a suitable name.

If it is safe to assume svelte isn't suddenly going to change the scoping implementation away from using css classes, i suggest naming it 'scopeClass' or 'cssScopeClass' .
Maybe add a little doc reminding users that they should return a reasonably short valid css classname.

@Rich-Harris
Copy link
Member

I think I like cssHash — it's short, to the point, and implementation-agnostic. (If you use it to generate svelte-foo and svelte-bar based on the filename, those aren't really 'hashes', but I think that's probably fine)

@kaisermann kaisermann force-pushed the feat/allow-custom-scope-class branch from d8b87a1 to 66caecd Compare February 17, 2021 20:33
@kaisermann
Copy link
Member Author

@Rich-Harris Done! However, the current implemented cssHash method receives the filename, the component's name and the hash generated by the compiler's hash method.

image

Did you mean for it to only receive the filename?

@dominikg
Copy link
Member

Thanks for updating this @kaisermann

I think filename and component name are good to have. One thing i'm wondering is if it is worth it to pass in the raw styles instead of a precalculated hash and leave hashing completely to the implementation of cssHash. The call of the default hash() function would then happen in the default implementation of cssHash. That way a custom implementation would not have to pay the (arguably not very high) cost of the default css content hash.

@kaisermann
Copy link
Member Author

kaisermann commented Feb 18, 2021

Yeah, that's exactly my reasoning behind my last comment above 🙈. If I remember correctly, the initial idea was to be able to easily customize the "svelte-" prefix.

this.source = source;
this.ast = ast;
this.filename = filename;
this.dev = dev;

if (ast.css && ast.css.children.length) {
this.id = `svelte-${hash(ast.css.content.styles)}`;
this.id = get_css_hash({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should add a saftynet here and warn the user if he is passing in a hash that is none compliant (similar to a11y checks).

@dreitzner
Copy link
Contributor

I am really looking forward to this being shipped ☺
I think we'll break svelte detection for most of the website tools out there, if I'm not mistaken 🤣

@Rich-Harris
Copy link
Member

One thing i'm wondering is if it is worth it to pass in the raw styles instead of a precalculated hash and leave hashing completely to the implementation of cssHash

Alternatively: what if cssHash was passed the hashing function itself? That way we don't need to generate the hash ourselves, if cssHash is provided, but it also provides a very convenient way for custom cssHash functions to use that logic for, say, hashes based on filenames:

compilerOptions: {
  filename,
  cssHash: ({ hash, name, filename, css }) => {
    // default behaviour
    return `svelte-${hash(css)}`;

    // stable hashes for HMR
    return `svelte-${hash(filename)}`;
  }
}

@Rich-Harris Rich-Harris mentioned this pull request Feb 25, 2021
@Rich-Harris
Copy link
Member

Opened a PR for that: #6026

@dominikg
Copy link
Member

great!. we should check that feeding the hash function a lot of similar strings (iE file paths within the same root) that it doesn't produce collisions. but even if that is the case a custom impl can just ignore the provided hash fn and roll its own. I can't wait to have this released :D

@kaisermann
Copy link
Member Author

kaisermann commented Feb 26, 2021

Closing this since #6026 was merged 🎉

@kaisermann kaisermann closed this Feb 26, 2021
@kaisermann kaisermann deleted the feat/allow-custom-scope-class branch February 26, 2021 11:24
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.

7 participants