hudochenkov / stylelint-order

A plugin pack of order related linting rules for Stylelint.
MIT License
916 stars 61 forks source link

Properties-alphabetical-order mixin error within styled-components #63

Closed ekfuhrmann closed 1 year ago

ekfuhrmann commented 6 years ago

With the following component:

const Foo = styled.p`
   margin: 0;
   text-transform: uppercase;

  ${font("sans-serif", 14, 600, #000)};
`;

I receive this error:

Expected -styled-mixin0 to come before text-transform (order/properties-alphabetical-order)

The mixin I have is simply creating some font style properties based off of the parameters. It seems like the alphabetical ordering is using the word mixin and thus wants it placed above text-transform but after margin.

hudochenkov commented 6 years ago

Sorry for a late response.

This plugin doesn't know anything about styled components. From what I see in your report, you're probably using stylelint-processor-styled-components. Please, refer to this issue https://github.com/styled-components/stylelint-processor-styled-components/issues/117 for solution.

ost-ing commented 6 years ago

I'm facing the same issue. Would it make sense to add an option to ignore the sorting when ${font('sans-serif', 14, 600, #000)}; is detected? E.g. ignore ${ or something similar?

Imo it would be handy

nickserv commented 6 years ago

Can we please reopen this issue for a workaround, since stylelint-processor-styled-components can't be changed to improve this?

I think the best we can do is automatically ignore rules starting with -styled-.

hudochenkov commented 6 years ago

@nickmccurdy stylelint supports CSS-in-JS out of the box now. Have you tried it without using stylelint-processor-styled-components?

nickserv commented 6 years ago

I removed "processors": "stylelint-processor-styled-components", and my linter errors are no longer working on Stylelint 9.6.0

taylon commented 5 years ago

It is weird that every Stylelint rule I tested so far works in CSS-in-JS (including ---fix) out of the box. Except for stylelint-order. The order rules don't report any errors in CSS-in-JS.

hudochenkov commented 5 years ago

@taylon please, create a separate issue with a description of your problem.

markcellus commented 5 years ago

Definitely having a similar problem as @nickmccurdy here. I realize this may not be a direct issue in this repository but it does seem odd that this lib can't ignore styled-component mixins. See code below:

import styled, { css } from 'styled-components';

export const buttonReset = css`
    background: none;
    border-bottom: 0;
    font: inherit;
    padding: 0;
`;

export const StyledButton = styled.button`
    ${buttonReset}
    border: 1px solid red;
}

And I get this in the console:

Expected border to come before -styled-mixin0   order/properties-alphabetical-order

Looks like its expecting the mixin variable ${buttonReset} to come after the border property, but I need for the mixin variable to be first in order to override it.

markcellus commented 5 years ago

sorry I meant that issue is the same one that @ekfuhrmann originally had. Can we reopen this issue and explore a possible fix?

hudochenkov commented 5 years ago

stylelint-order works great with Styled Components. I use it every day. Don't use stylelint-processor-styled-components and you won't see any warnings like above.

markcellus commented 5 years ago

Sure, but as @nickmccurdy has already pointed out, stylelint-processor-styled-components can't be removed because linting stops working altogether. Using latest version of stylelint: 10.0.1.

Also, just to clarify. These are not warnings, they're actual errors that fail the lint tests.

hudochenkov commented 5 years ago

These are not warnings, they're actual errors that fail the lint tests.

Please, create an issue in stylelint if this happens. Help us fix these errors.

I have no intention to support stylelint-processor-styled-components as this plugin work without any stylelint processors.

markcellus commented 5 years ago

😕 This is the same error that was originally brought up in the thread. Why not just reopen this?

markcellus commented 5 years ago

I have no intention to support stylelint-processor-styled-components as this plugin work without any stylelint processors.

Hmm, don't think anyone is asking for you to support stylelint-processor-styled-components. We're asking that you support use of ${} expressions and ignore them. You don't technically need stylelint-processor-styled-components to use ${} expressions in your css-in-js.

hudochenkov commented 5 years ago

The original error is caused by using stylelint-processor-styled-components. It won't be fixed.

I asked you to not use this processor, and you said that there are other errors. Please, report these errors to stylelint, so we can fix it.

markcellus commented 5 years ago

The original error is caused by using stylelint-processor-styled-components. It won't be fixed.

See my previous comment. It's not caused by the use of stylelint-processor-styled-components. This is an issue caused by using ${} expressions in your styled-components when using this plugin.

I asked you to not use this processor, and you said that there are other errors.

😕 never said there are any other errors than those that are already mentioned here.

hudochenkov commented 5 years ago

I think there is misunderstanding from both sides.

Warning like this:

Expected -styled-mixin0 to come before text-transform (order/properties-alphabetical-order)

Is definitely caused by stylelint-processor-styled-components because -styled-mixin0 created from this processor. It's not from stylelint itself.

Try to remove stylelint-processor-styled-components from your stylelint config and see what happens. It should work as expected. I just tried your example and didn't get any warnings, because order is correct. Then I changed order in the first component and got warnings, because order was incorrect.

markcellus commented 5 years ago

Then I changed order in the first component and got warnings, because order was incorrect.

This will be my last comment about this since I'm pretty busy. Your statement above is incorrect. It looks like everything is fine because stylelint stops discovering other errors altogether and no longer works properly. Use my example again, remove stylelint-processor-styled-components, and put some other error there (maybe like duplicate selector) and you will see that that won't produce any error also.

drwpow commented 5 years ago

I have a dumb hack for this, that doesn’t really fix the problem but at least suppresses the error in many scenarios. This works because the list from css-known-properties out-of-the-box comes alphabetized with vendor prefixes, as you’d expect.

// .stylelintrc.js

const properties = require('known-css-properties').all;

module.exports = {
  processors: ['stylelint-processor-styled-components'],
  plugins: ['stylelint-order'],
  rules: {
    'order/properties-order': [...properties, '-styled-mixin0'], // alphabetize, but put Styled Components last

The real problem is that certain properties used by Styled Components, et al shouldn’t be alphabetized at all—they either need to come first or last depending on the situation. Anyway, what I’d like to see with this plugin would be an ignore option that would let you whitelist things like -styled-mixin0, and in the sorting algorithm would just leave this as-is wherever it’s found.

I could open a PR for this if this is desirable to others.

hudochenkov commented 5 years ago

@dangodev why stylelint-processor-styled-components is used? stylelint supports styled components out of the box without the need for processor.

drwpow commented 5 years ago

@hudochenkov I’m sorry—I’ve read this entire thread, and I know you keep saying that, and feel like you’re being ignored. But that doesn’t seem to be the case. As of stylelint 10.1.0 and Styled Components 5.0.0-beta.3 it does not work without the processor. This is what I see:

Screen Shot 2019-06-18 at 17 10 58
Expected empty line before declaration (declaration-empty-line-before)stylelint(declaration-empty-line-before)

Not only does that error appear in every Styled Components block, but without the processor, all the other lint rules stop working (such as alphabetization). That’s trying to run it without stylelint-processor-styled-components.

What’s more, the Styled Components documentation still recommends using the processor, and the repo is still actively maintained. It’s hard to see how to use Styled Components in Stylelint without this.

hudochenkov commented 5 years ago

@dangodev would you mind opening an issue in stylelint repository about declaration-empty-line-before issue, please? It's either bug or incorrect config.

If you have examples of other core rules, please, open issues in stylelint repository as well. Help us make stylelint better :)

markcellus commented 5 years ago

@dangodev either he's just not getting it or all of us are just doing a very very bad job explaining this lol.

hudochenkov commented 5 years ago

I'm getting it. styled-components processor were introduced long before stylelint was supporting CSS-in-JS. Now there are less reasons to use processor. There are some problems which developers of stylelint don't know about, because users don't report them.

What I see from this thread, that users facing problems with built-in support, so they choose quick solution — use processor, and users happy unless process backfires sometimes (like this issue). It's a workaround to a problem and a short term solution. I'm asking to invest a little bit of time to long term solution — report issues you're encountering to stylelint repository. Help us to help you and make better tool for everyone.

drwpow commented 5 years ago

What confused us was the messaging here. I see issues like this one that are working toward deprecating processors. And I’m all for it! Love the idea of Stylelint supporting CSS-in-JS natively 🎉!

We can (and will) open up tickets, for sure. But when you say “it supports,“ that’s not accurate because for (arguably) the most popular CSS-in-JS library, Styled Components, it is clearly and obviously broken. It is lacking the most basic of support. Let’s just level with the fact that it doesn’t work, and the official Styled Components documentation still recommending you to use the processor agrees with this.

Saying “it works now” isn’t correct. But saying “we’re moving toward deprecating processors, and want to be able to support Styled Components without it” is awesome! 💯 All in favor of working for that solution. Hopefully that clarifies the disconnect here.

hudochenkov commented 5 years ago

For a note: I use styled components for half a year now with stylelint and autofixing. I use stylelint-config-recommended and order/order and order/properties-order with autofixing.

That where from some of my confusion comes. I use both and don't have problems (almost :)).

hudochenkov commented 5 years ago

While fixing everything in stylelint could take a while, users want to continue using processor in the meanwhile.

Consider contributing a fix to ignore -styled-* things.

ekfuhrmann commented 5 years ago

@hudochenkov would you mind sharing your stylelint config that you use for React?

hudochenkov commented 5 years ago

@ekfuhrmann I use two configs. One default, which is also picked up by editor plugin — .stylelintrc.js. And the second which extends default config and add more things that executed on precommit hook — .stylelintrc-fix.js.

stylelint "src/**/*.styled.js" --config .stylelint-fix.rc.js
// .stylelintrc.js
module.exports = {
    extends: ['stylelint-config-recommended'],
    plugins: ['stylelint-order'],
    rules: {
        'no-empty-source': null,
        'order/order': [['declarations', 'rules']],
    },
};
.stylelintrc-fix.js (click to expand) ```js // .stylelintrc-fix.js module.exports = { extends: './.stylelintrc.js', rules: { 'order/properties-order': [ [ { emptyLineBefore: 'always', noEmptyLineBetween: true, properties: [ 'content', 'position', 'top', 'right', 'bottom', 'left', 'z-index', 'display', 'vertical-align', 'visibility', 'flex', 'flex-grow', 'flex-shrink', 'flex-basis', 'flex-direction', 'flex-flow', 'flex-wrap', 'grid', 'grid-area', 'grid-template', 'grid-template-areas', 'grid-template-rows', 'grid-template-columns', 'grid-row', 'grid-row-start', 'grid-row-end', 'grid-column', 'grid-column-start', 'grid-column-end', 'grid-auto-rows', 'grid-auto-columns', 'grid-auto-flow', 'grid-gap', 'grid-row-gap', 'grid-column-gap', 'align-content', 'align-items', 'align-self', 'justify-content', 'order', 'float', 'clear', 'overflow', 'overflow-x', 'overflow-y', 'overflow-scrolling', 'clip', 'box-sizing', ], }, { emptyLineBefore: 'always', noEmptyLineBetween: true, properties: [ 'width', 'min-width', 'max-width', 'height', 'min-height', 'max-height', 'margin', 'margin-top', 'margin-right', 'margin-bottom', 'margin-left', 'padding', 'padding-top', 'padding-right', 'padding-bottom', 'padding-left', 'border', 'border-spacing', 'border-collapse', 'border-width', 'border-style', 'border-color', 'border-top', 'border-top-width', 'border-top-style', 'border-top-color', 'border-right', 'border-right-width', 'border-right-style', 'border-right-color', 'border-bottom', 'border-bottom-width', 'border-bottom-style', 'border-bottom-color', 'border-left', 'border-left-width', 'border-left-style', 'border-left-color', 'border-radius', 'border-top-left-radius', 'border-top-right-radius', 'border-bottom-right-radius', 'border-bottom-left-radius', 'border-image', 'border-image-source', 'border-image-slice', 'border-image-width', 'border-image-outset', 'border-image-repeat', 'border-top-image', 'border-right-image', 'border-bottom-image', 'border-left-image', 'border-corner-image', 'border-top-left-image', 'border-top-right-image', 'border-bottom-right-image', 'border-bottom-left-image', ], }, { emptyLineBefore: 'always', noEmptyLineBetween: true, properties: [ 'background', 'background-color', 'background-image', 'background-attachment', 'background-position', 'background-position-x', 'background-position-y', 'background-clip', 'background-origin', 'background-size', 'background-repeat', 'color', 'box-decoration-break', 'box-shadow', 'outline', 'outline-width', 'outline-style', 'outline-color', 'outline-offset', 'table-layout', 'caption-side', 'empty-cells', 'list-style', 'list-style-position', 'list-style-type', 'list-style-image', ], }, { emptyLineBefore: 'always', noEmptyLineBetween: true, properties: [ 'font', 'font-weight', 'font-style', 'font-variant', 'font-size-adjust', 'font-stretch', 'font-size', 'font-family', 'src', 'line-height', 'letter-spacing', 'quotes', 'counter-increment', 'counter-reset', '-ms-writing-mode', 'text-align', 'text-align-last', 'text-decoration', 'text-emphasis', 'text-emphasis-position', 'text-emphasis-style', 'text-emphasis-color', 'text-indent', 'text-justify', 'text-outline', 'text-transform', 'text-wrap', 'text-overflow', 'text-overflow-ellipsis', 'text-overflow-mode', 'text-shadow', 'white-space', 'word-spacing', 'word-wrap', 'word-break', 'overflow-wrap', 'tab-size', 'hyphens', 'interpolation-mode', ], }, { emptyLineBefore: 'always', noEmptyLineBetween: true, properties: [ 'opacity', 'filter', 'resize', 'cursor', 'pointer-events', 'user-select', ], }, { emptyLineBefore: 'always', noEmptyLineBetween: true, properties: [ 'unicode-bidi', 'direction', 'columns', 'column-span', 'column-width', 'column-count', 'column-fill', 'column-gap', 'column-rule', 'column-rule-width', 'column-rule-style', 'column-rule-color', 'break-before', 'break-inside', 'break-after', 'page-break-before', 'page-break-inside', 'page-break-after', 'orphans', 'widows', 'zoom', 'max-zoom', 'min-zoom', 'user-zoom', 'orientation', 'fill', 'stroke', ], }, { emptyLineBefore: 'always', noEmptyLineBetween: true, properties: [ 'transition', 'transition-delay', 'transition-timing-function', 'transition-duration', 'transition-property', 'transform', 'transform-origin', 'animation', 'animation-name', 'animation-duration', 'animation-play-state', 'animation-timing-function', 'animation-delay', 'animation-iteration-count', 'animation-direction', 'animation-fill-mode', ], }, ], { unspecified: 'bottom', }, ], }, }; ```
mulholo commented 4 years ago

Has anyone got a hack/solution for globally ignoring errors which contain -styled-x? I don't want to do an ignore comment every time.

e.g.

[stylelint order/properties-alphabetical-order] [E] Expected -styled-mixin1 to come before transform (order/properties-alphabetical-order)
saiichihashimoto commented 3 years ago

It's been close to 2 years since @hudochenkov last commented on this. Is there a way we can move forward? Currently, using styled components and using this rule are mutually exclusive.

ghost commented 3 years ago

I had the same issue, disabling "stylelint-processor-styled-components" make this plugins works but in other hand indentation rule starting to had inconsistent behavior, with "stylelint-processor-styled-components" order/properties-alphabetical-order doesn't had the issue related with mixing, so in the end I prefer have the indentation working properly and I use https://github.com/tinloof/eslint-plugin-better-styled-components to get alphabetical-order rule until the indentation be fixed without require the processor

hudochenkov commented 1 year ago

Closing as stylelint-processor-styled-components was deprecated. Use postcss-styled-syntax custom syntax instead.