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

Option to use only css modules without $style. prefix #13

Closed
non25 opened this issue Mar 20, 2021 · 13 comments
Closed

Option to use only css modules without $style. prefix #13

non25 opened this issue Mar 20, 2021 · 13 comments

Comments

@non25
Copy link
Contributor

non25 commented Mar 20, 2021

Hi.

Amazing project.

Recently I started to really getting fed up with svelte's default style scoping, mostly due to it changing selectors weight thus making some intuitive css overrides unintuitive and the inability to override some leaf-component styles without wrapping it with divs...

Maintainers obsession with css properties and div wrapping is beyond me.

I like SFC format and the terseness of class="some-class" in the template in comparison to something like class="{style.someClass}".

What do you think about parsing to AST using svelte internal parser like it is done in svelte-trim and more safely changing classes in the markup through that ?

It would enable to safely omit $style. prefixes and get more "native" experience. 😁

In a perfect world that takes maintainers attitude towards other scoping mechanisms, or stripping whitespace I would love to see css modules included in svelte-preprocess to increase adoption and make it a drop-in replacement for svelte's native style scoping.

Another day one sad man in svelte chat wanted to strip base classes from a widget "scoped" styles to prevent outer page affecting the widget... Sent him a link to your preprocessor. 🙂

What do you think? 🤔

@micantoine
Copy link
Owner

micantoine commented Mar 28, 2021

Hi @non25 thank you for your feedback.

What do you think about parsing to AST using svelte internal parser

Definitely, using the svelte parser and walker would be smarter and less prone to errors.

It would enable to safely omit $style. prefixes

The use of the prefix $style. was to differentiate cssModules classnames from global and scoped classnames. When making the preprocessor I thought it would be nice to still have the option to use the default svelte scoped system on classes. Perhaps it doesn't have to be that way as it would be less likely for a developer to use both method (scoped class and modules class) within the same component.

Thus, we could add an attribute to <style> to imply the use of module over scoped and then only replace the non global classes.

increase adoption and make it a drop-in replacement for svelte's native style scoping.

I don't think the native scoping will be dropped. It does work well on a full svelte application and has the advantage to allow the use of simple html tag as selector instead of adding a class on every single element. (Except that part though sveltejs/svelte#4374)

to prevent outer page affecting the widget.

Yes, that was the main reason for creating the preprocessor. Class inheritance on hybrid project.


To summarize, getting rid of $style. is definitely something to focus on for v2 which may imply the addition of the attribute module to <style>. The default scoped system will remain on non-class selectors.

<p class="serif externalClass">My lorem ipsum paragraph</p>
<p class="green serif">My green lorem ipsum paragraph</p>

<style module>
  p {
    font-size: 18px;
  }
  :global(.serif) {
    font-family: serif;
  }
  .green { color: green }
</style>

generating

<p class="serif externalClass svelte-vg78j0">My lorem ipsum paragraph</p>
<p class="green-1ErDT serif svelte-vg78j0">My green lorem ipsum paragraph</p>

<style>
  p.svelte-vg78j0 {
    font-size: 18px;
  }
  .serif {
    font-family: serif;
  }
  .green-1ErDT { color: green }
</style>

Anything else we should consider ?

@non25
Copy link
Contributor Author

non25 commented Mar 28, 2021

I don't think the native scoping will be dropped.

I didn't mean it would be dropped. I meant that a user could install svelte-preprocess-cssmodules, make simple config adjustments, preferably inside svelte-preprocess and use it the same way as svelte scoping, but with classes and nested tag selectors like .some-class p.

(Except that part though sveltejs/svelte#4374)

That part, and the part where you try to patch global classes from within the component without overriding too much. Like svelte-scoped .btn overriding global .btn.warning, btn:hover or something like that.

The default scoped system will remain on non-class selectors.

There should be an option to output selectors wrapped with globals when using <style module>.

Here's the problem with the approach you are trying to stick to.

<p class="green">My green lorem ipsum paragraph</p>

<style module>
  p {
    color: red;
    font-size: 18px;
  }

  .green {
    color: green;
  }
</style>
<p class="green-1ErDT">My green lorem ipsum paragraph</p>

<style>
  p.svelte-vg78j0 {
    /* now this selector will have more weight than .green-1ErDT */
    color: red;
    font-size: 18px;
  }

  .green-1ErDT {
    /* this rule will get overriden */
    color: green;
  }
</style>

Also .green-1ErDT { color: green } should be generated as :global(.green-1ErDT) { color: green } to prevent svelte scoping on already scoped class.

So to conclude, the only acceptable way of doing it that I see now is:

Look for class selectors in the style tag,
then look for classes in elements: <div class="some-class">, components: <Component class="some-class" />,
then scope both and wrap everything in :global() to prevent svelte-scoping from interfering.

I'd like to help you achieving that and then propose to svelte-preprocess to include configuration for this preprocessor. Could use some hints on where to start.

Thanks for response. 🙂

@micantoine
Copy link
Owner

Like svelte-scoped .btn overriding global .btn.warning

Yes that makes sense, if you add a class .btn inside your component <style> it's in general to add/override some styles of that global class .btn for that particular component. I'm note sure to understand your issue here ? Are you completely changing the color of it ? and then applying .warning to a.btn element ? It seems to be more a problem with the styling architecture approach.

There should be an option to output selectors wrapped with globals when using <style module>

Like you would prefer to have every selectors treated as global instead of using the provided solution :global()? Svelte scopes everything within the component to avoid unexpected inheritances.

This will happen if every tag selectors are treated as global.

<!-- Parent Component -->

<div class="myClass">
  <h3>Loren Epsom Title</>
  <p>Lorem ipsum paragraph</p>

  <ChildComponent />
</div>

<style module>
  .myclass p {
    font-size: 20px;
    font-weight: bold;
    text-transform: uppercase;
  }
</style>

<!--- Child Component --- >

<div class="child">
   <p>A child lorem ipsum paragraph</p>
</div>

<style module>
  /* Overrides parent component styles */
  .child p {
    font-size: 16px;
    font-weight: regular;
    text-transform: none;
  }
</style>

It is something to consider (and yes CSS Modules works that way css-modules/css-modules#131)

Here's the problem with the approach you are trying to stick to...

Yes I know, tag selectors have more weight than a css modules className because they are scoped with a class. I can see it happening

<ul>
 <li>Home</li>
 <li class="active">About</li>
</ul>

<style module>
 li {
   color: gray; 
 }
 /* this will never be applied */
 .active {
   color: blue;
 }
 /* forcing us to that that instead */
 li.active {
   color: blue;
 }
</style>

or rewriting the component such as

<ul>
 <li class="item">Home</li>
 <li class="item active">About</li>
</ul>

<style module>
 .item {
   color: gray; 
 }
 .active {
   color: blue;
 }
</style>

I believe it's just the way we style our components. The CSS modules philosophy is to apply a class to every elements requiring styling. A tag selector is always global or within the scoped of its parent class.

As I match the functionality of the preprocessor to the way I style my components, I kept the native scoping so that I could use the approach I wanted.

Also .green-1ErDT { color: green } should be generated as :global(.green-1ErDT) { color: green } to prevent svelte scoping on already scoped class.

Yes, this is already happening

In conclusion, It would definitely be nice to have the full CSS Modules approach available for developers familiar with it as well as the option to mix it with scoped tag for other developers who want it.

Adding a global settings mode to the preprocessor would solve that.

Mode native generating

<p>Lorem ipsum paragraph</p>
<p class="red-Bf90i">Red lorem ipsum paragraph</p>

<style>
 p {
   font-style: italic;
 }
 .red-Bf90i {
   color: blue;
 }
</style>

Mode mixed generating

<p class="svelte-we78ty">lorem ipsum paragraph</p>
<p class="red-Bf90i svelte-we78ty">Red lorem ipsum paragraph</p>

<style>
 p.svelte-we78ty {
   font-style: italic;
 }
 .red-Bf90i {
   color: blue;
 }
</style>

And maybe even a third mode scoped that would fix the weighting issue while making the classname unique to avoid inheritance.

<p class="svelte-we78ty">lorem ipsum paragraph</p>
<p class="red-Bf90i svelte-we78ty">Red lorem ipsum paragraph</p>

<style>
 p.svelte-we78ty {
   font-style: italic;
 }
 .red-Bf90i.svelte-we78ty {
   color: blue;
 }
</style>

We could also add the possibility to override the global settings by doing it locally <style module="mixed">

The version 2 of the preprocessor will then include

  • Drop of the $style. prefix
  • Use of the svelte parser and walker (AST)
  • New Global settings option mode (native, mix, scoped ?)
  • Requirement of the module attribute on style
  • External stylesheet import still available (to be parsed with AST)

I'd like to help you achieving that

I appreciate that, unfortunately at this stage, I don't think we can both work simultaneously on it, splitting tasks, as the library is very small and everything is connected (basically redoing the parsing), we would be coding the same things on the same files. I will have do to some preparation first, I will let you know.

Your inputs and suggestions were already helpful, thank you for that.

In the meanwhile, if you follow the cssModules approach of a class on every elements, you shouldn't encounter overriding issues.

@non25
Copy link
Contributor Author

non25 commented Mar 30, 2021

I'm note sure to understand your issue here ?

https://svelte.dev/repl/90ac3d2588644eedb81bde4c24b11051?version=3.36.0

Here's an example, I'm not sure you will find it reasonable.

Like you would prefer to have every selectors treated as global instead of using the provided solution :global()? Svelte scopes everything within the component to avoid unexpected inheritances.

Exactly. I'm used to css cascade, but I don't want to get used to unexpected selector weight wars. 🙂

In conclusion, It would definitely be nice to have the full CSS Modules approach available for developers familiar with it as well as the option to mix it with scoped tag for other developers who want it.

Yeah, options never hurt. I would expect native scoping to be the default one.

* New Global settings option **mode** (`native`, `mix`, `scoped` ?)

That's a hard one to name. I can't think of something shorter than native, mixed, svelte-scoped.
Something like <style module svelte-scope="tags|everything"> looks more self-explainatory... and more ugly.

And now I came to a realization that I wouldn't be able to override child-component styles like .button with something like .button.custom, because it will be .button-a3fj2s and just targeting .custom wouldn't be enough, because button styles come last. I could use something stupid like .custom.custom.

Interestingly enough, that behavior is altered when toggling emitCss in svelte options... With true parent styles will come last, and with false parent styles will come first, because styles are inserted on instantiation.

I guess the way to style button components correctly would be .button.primary or .button.regular. 🤔 That would fix the issue of overriding .btn.busy, or :hover.

I like your proposed way of doing it, however I don't see myself using mixed or scoped.

I guess I'm out of useful input. 🙂

@micantoine
Copy link
Owner

I made a first draft which can be installed from npm install --save-dev svelte-preprocess-cssmodules@next
It is following what we talked about. The native mode is enabled by default, so no need to change the settings in your case. Otherwise the mode can be globally set from the preprocessor option, or locally per component from the module attribute.

The Readme is still very minimum yet, I will work on it.

For your issue regarding the button, not sure the native mode will solve it by itself, you may have to chain the modifier classes like you said.

@non25
Copy link
Contributor Author

non25 commented Apr 12, 2021

Wow, this is so cool. 😀

I've tried it out and as far as I tested, for me this implementation is 3 steps away from being perfect. 🙂

  1. Use component path as a seed for component-scoped classNames. Currently it looks like they are random for every class. This will make class replacement through HMR so much easier.
  2. Scope classes sent to a component through class prop? Perhaps to avoid conflicts allow to set prop name in the options for component class-passing? https://github.com/non25/svelte-template-webpack/blob/css-modules-micantoine/src/App.svelte
  3. Somehow fix class:something shorthands? I'll look for myself what are the blockers for this.

Thank you! This is amazing.

Edit: read readme more carefully and it looks like I can use component path as a seed just by modifying options like it is done in webpack. 🤔

@non25
Copy link
Contributor Author

non25 commented Apr 13, 2021

External classes are scoped, but it looks like styles aren't being applied. 🤔

https://github.com/non25/svelte-template-webpack/tree/css-modules-micantoine

@micantoine
Copy link
Owner

  1. Use component path as a seed for component-scoped classNames. ....

If as a seed, you mean to use it within the className , yes you can set your own localIndentName

  1. Scope classes sent to a component through class prop?

That is something I didn't think of, I'm checking every node type Element during the parsing, but components are treated differently (InlineComponent). A tweak need to be work on.

  1. Perhaps to avoid conflicts allow to set prop name in the options for component class-passing?

Please, can you explain more what you have in mind, I don't understand here

3 Somehow fix class:something

Basically class directive shorthand cannot have dynamic value, so when transforming the directive classname into css modules, it doesn't match the variable name anymore.
That made me think of a trick we could do. We should be able to transform class:something into class:something-ETYer={something} during the parsing,

External classes are scoped, but it looks like styles aren't being applied.

This is a bug from the external import along with the native mode. Need to work on a fix.

Thank for your tests.

@non25
Copy link
Contributor Author

non25 commented Apr 13, 2021

If as a seed, you mean to use it within the className , yes you can set your own localIndentName

I wanted something like .class-Bf90i where Bf90i part is completely static for every class of one component, and is dependend on a filepath. That way, we can swap only css with HMR, and it will work itself out, because classnames didn't change. Details.

Please, can you explain more what you have in mind, I don't understand here

There's a myth that somebody somewhere uses component class prop for something other than css classes. 😁
I don't believe that is even rarely a case, so I would just scan class prop of an InlineComponent and hash its contents if corresponding selectors are found in the style tag. 🙂
However, an option to choose which component prop to scan for classes to hash them, for example: className won't hurt anyone. 🤔

That made me think of a trick we could do. We should be able to transform class:something into class:something-ETYer={something} during the parsing,

That is clever. Cool. 👍

Thank for your tests.

Thank you for the great work. 🙂

@micantoine
Copy link
Owner

micantoine commented Apr 16, 2021

New version available npm i [email protected] or npm i svelte-preprocess-cssmodules@next

Check the readme for further details

Features

  • Add option hashSeeder to customize the source of the hashing method
  • Add option allowedAttributes to parse other attributes than class

Fixes

  • Replace class attribute on HTML elements and inline components
  • Fix external import on native & mixed mode when <style> already exists
  • Shorthand directive

Example of use

hashSeeder option

// Preprocess config
...
preprocess: [
  cssModules({
    hashSeeder: ['filepath'],
  })
],
...
<button class="success">Ok</button>
<button class="cancel">Cancel</button>
<style module>
  .success { background-color: green; }
  .cancel { background-color: gray; }
</style>

Generating

<button class="success-yr6RT">Ok</button>
<button class="cancel-yr6RT">Cancel</button>
<style>
  .success-yr6RT { background-color: green; }
  .cancel-yr6RT { background-color: gray; }
</style>

allowedAttributes option

// Preprocess config
...
preprocess: [
  cssModules({
    allowedAttributes: ['data-color', 'classname'],
  })
],
...
<button class="red" data-color="red">Red</button>
<button class="red" classname="blue">Red or Blue</button>
<style module>
  .red { background-color: red; }
  .blue { background-color: blue; }
</style>

Generating

<button class="red-yr6RT" data-color="red-yr6RT">Red</button>
<button class="red-yr6RT" classname="blue-aE4qW">Red or Blue</button>
<style>
  .red-yr6RT { background-color: red; }
  .blue-aE4qW { background-color: blue; }
</style>

@non25
Copy link
Contributor Author

non25 commented Apr 16, 2021

Yeah, I've been monitoring and building v2 since yesterday. 😁
Now the preprocessor is perfect. 🥇
Do you want to wait for some time to make sure there are no unexpected stuff before releasing? 🤔

@micantoine
Copy link
Owner

I fixed few other bugs from the the native parsing after doing further testing.

I also added another feature :local() that could come handy in some situation (Basically doing the opposite of :global() read here)

New version npm i [email protected]

@micantoine
Copy link
Owner

Fixed in v2

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

No branches or pull requests

2 participants