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

Keep empty selectors (rulesets) option #1006

Open
vladcosorg opened this issue Oct 28, 2012 · 28 comments
Open

Keep empty selectors (rulesets) option #1006

vladcosorg opened this issue Oct 28, 2012 · 28 comments

Comments

@vladcosorg
Copy link

Can you add please an options to keep empty selectors, or at least point me at the file where i can add it myself. I need it to ease the styling via firebug.

@Soviut
Copy link

Soviut commented Oct 28, 2012

Do you mean the :empty pseudo-selector?

http://reference.sitepoint.com/css/pseudoclass-empty

@vladcosorg
Copy link
Author

No, probably i should have called it a "rule" ;)
I mean this

.selector {}
a.nother{
   .selector {}
}

They get deleted on compiling to css

@Soviut
Copy link

Soviut commented Oct 28, 2012

Why would you want to keep them? They're not doing anything lying around blank in the generated CSS file. You say you're trying to do styling via Firebug, but I don't understand what your approach is.

@vladcosorg
Copy link
Author

  1. I create empty rules in the less file for an element or a group of elements.
  2. I open firebug, select the element i intend to work on, and I can see the empty rules i created
  3. I write styles inside that empty rule
  4. The https://github.com/ronniekk/css-x-fire plugin syncs back the changes i made in firebug to less file, it finds the empty rule, and puts there the styles i specified in firebug.

The requirement is that the rule have to already exist in order for the plugin to sync back correctly, and obviously, if the element hasn't had styles before, i'll have to create a empty rule for the first time. It worked when i used pure CSS, but now, when i switched to LESS, i realized that compiler deletes the empty rules, and the plugin is unable to sync back the styles.

I realize that that's a pretty narrow use-case...

@Soviut
Copy link

Soviut commented Oct 28, 2012

This seems like a very specific use case. In most cases, people would rather LESS optimized their CSS and didn't leave empty rules.

I would suggest using less.watch() to automatically refresh your styles:

<script type="text/javascript">
     less.env = "development";
     less.watch();
</script>

or append #!watch to your URL.

If you have issues, there are some suggestions here: http://www.paulsprangers.com/2011/04/quick-tip-less-js-in-development-and-watch-mode/

@lukeapage
Copy link
Member

Good advice from @Soviut. also if you don't like that, for work-around add a fake rule e.g.

temp: ruleset;

@Soviut
Copy link

Soviut commented Oct 28, 2012

Agreed. @agatronic 's solution does what you need without causing every other LESS file to generate inefficient CSS.

@vladcosorg
Copy link
Author

@agatronic that's what i was doing for the last 2 weeks since i started using LESS, but a few times such fake rules got into production because i forgot to remove them, so i thought maybe there is a more clear way to fix that :(

@Soviut unfortunately that's entirely different workflow, i'm not ready to give it up for using Less... also using less compiler in browser caused some serious performance issues, page load increased from 500-700ms to 2-3 s..

I understand that this option will not get added, and I agree with you, but If that's not a problem, can you tell me please in which source code file i can change the behavior by myself?

@lukeapage
Copy link
Member

@d4ng3rmax
Copy link

@chetzof I figured out the solution for this. that's no the BEST way, but works for what you looking for.

i have put that way:

#selector { /**/ }

it will appear empty on firebug.

@pengVc
Copy link

pengVc commented Nov 1, 2013

@d4ng3rmax Cool point!
i think i have the same dev workflow that @chetzof mentioned.
Thx.

@garkin
Copy link

garkin commented Jul 9, 2018

This thing is a must have for use during development with css-modules.
It's realy tiresome to boilerplate all the selectors during scaffolding and then manage to remove them.

.main {
/*! keep */
}

.loading {
/*! keep */
}

.button {
/*! keep */
}

.form {
/*! keep */
}

@matthew-dean
Copy link
Member

@garkin What's your reasoning / use-case for writing out your selectors like that with CSS modules?

@garkin
Copy link

garkin commented Jul 9, 2018

Otherwise they would be undefined during the import phase.

import * as React from 'react'
import * as cx from 'classnames';
import css from './home.less';

export class Home extends React.Component {
    render() {
        const isLoading = true;
        return <div className={cx(css.main, {
            [css.loading]: isLoading
         })}>
            Home
        </div>
    }
}

This leads to the runtime exceptions and ruins hot module replacement. Preventing selectors from removing fixes all those issues.
But during scaffolding you always need to keep in mind that selectors will be removed and you need to fight a compiler to prevent it. And then all those /*! keep */ comments need to be removed sometime in the future.

@matthew-dean
Copy link
Member

@garkin Hmm... isn't the answer just to finish writing your stylesheet? I'm saying it's a problem caused only by this development approach.

For example, where I work we use Less and Sass depending on the team, and in our current Sass build setup, empty selectors won't pass linting (the app won't compile). So I've been forced to just change my approach with CSS modules / React.

It's really this pattern that's the problem:

{
     [css.loading]: isLoading
}

If you changed to this pattern it wouldn't cause an exception:

<div className={`${isLoading && css.loading}`}></div>

In your example, you're attempting to define an object property with a name that may be undefined. If you switch the logic, your class can be undefined and no exception will be thrown.

@garkin
Copy link

garkin commented Jul 9, 2018

This so called "finish writing your stylesheet" requires a specific cognitive context and could take a significant amount of a time. It's much easier to be done after scaffolding step with markup and working HMR at hand.

This pattern is a dominant and a semi-official guideline to use, it was a part of the React distribution some time ago.
And this sample is obviously distilled. Your way is not readable and not scallable.

return <div className={cx(css.post, sharedCss.frame, {
    [css.support]: post.isSupport,
    [css.foreign]: showTranslation,
    [css.private]: post.isInternal,
    [css.cached]: post.status.isLoading
    ...
})}>...</div>

@garkin
Copy link

garkin commented Jul 9, 2018

CSS-modules is the primary approach to do styles this days and will be only more so in the future.
Stripping empty selectors - convolutedly changes code semantics when used with css-modules.

This behavior should be at least avoidable through configuration.

@matthew-dean
Copy link
Member

That's interesting. Re-opening not so much just for that use case but because Less shouldn't be in the business of "cleaning" CSS. The compress option was deprecated for similar reasons i.e. there are lots of maintained tools which will strip selectors / compress / add prefixes etc.

Likely this behavior was created when an empty selector was irrelevant to the browser, but it's somewhat valid that it's not irrelevant as data when you factor in CSS modules.

@matthew-dean
Copy link
Member

matthew-dean commented Jul 9, 2018

Actually, unless someone objects, I think this is safe to implement as an option. IMO it would be removeEmptyRulesets (not selectors) with a default value of true.

Edit: or should it be keepEmptyRulesets with a default value of false? 🤔 Probably the latter, since it makes for easier falsey checks when undefined.

@matthew-dean matthew-dean changed the title Keep empty selectors option Keep empty selectors (rulesets) option Jul 9, 2018
@rjgotten
Copy link
Contributor

when you factor in CSS modules

And it's not limited to those only. Also consider things like programmatic access via the CSSOM.

I'd say keepEmptyRulesets is a good option to add.
A wee bit on the verbose side, maybe. Not very nice for the CLI: --keep-empty-rulesets

@matthew-dean
Copy link
Member

matthew-dean commented Jul 11, 2018

A wee bit on the verbose side, maybe

I don't disagree, but do you have an alternate suggestion? It seems like a very specific behavior, so sometimes it helps to be explicit. There's nothing stopping it from being keepEmptyRulesets in API and --keep-rulesets in CLI. Or even --keep-empty

@matthew-dean
Copy link
Member

Should it just be keepEmpty for both?

@rjgotten
Copy link
Contributor

rjgotten commented Jul 11, 2018

I would use:

  1. outputEmptyRulesets : true|false as the API option;
  2. --empty-rulesets as the full-form CLI toggle; and
  3. -er or possibly -empty as the shorthand CLI toggle.

@matthew-dean
Copy link
Member

@rjgotten I'm okay with that. I'm not emotionally invested lol. @garkin - how do you feel about this?

@garkin
Copy link

garkin commented Jul 11, 2018

Looks fine for me.

@orchidoris
Copy link

When we can expect for actual implementation of it?
This is an issue for us too.

@matthew-dean
Copy link
Member

@orchidoris PRs welcome!

@masell
Copy link

masell commented Mar 24, 2020

Workaround plugin...

Adds __NOOP__: 1; to every selector, then removes them after less is done.

class NoopPreProcessor {      
    process = (css, extra) => css.replace(/}/g, ';__NOOP__: 1;}');                                                                      
}      

class NoopPostProcessor {      
    process = (css, extra) => css.replace(/__NOOP__: 1;/g, '');                                                                               
}                                                                                                                       
  
const NoopPlugin = {                                                                                                    
    install: (less, pluginManager) => {                             
        pluginManager.addPreProcessor(new NoopPreProcessor());        
        pluginManager.addPostProcessor(new NoopPostProcessor());      
    },                                                                
}; 

                                                                   

For preact with less-loader:

    helpers.getLoaders(config)                                                             
        .filter(item => item.ruleIndex===1)      
        .map(item => {                           
            item.loaders[0].options.options.stictMath = true;      
            item.loaders[0].options.options.plugins = [            
                NoopPlugin,                                        
            ];                                                     
                                                             
            item.loaders[0].options.options.paths = [      
                ...item.loaders[0].options.options.paths[0],      
                path.resolve(__dirname, 'src'),                   
            ];                                                    
        });                                                       

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

9 participants