Skip to content
This repository has been archived by the owner on May 10, 2023. It is now read-only.

Error with nested rules #41

Closed
ai opened this issue Jul 9, 2015 · 11 comments
Closed

Error with nested rules #41

ai opened this issue Jul 9, 2015 · 11 comments

Comments

@ai
Copy link
Contributor

ai commented Jul 9, 2015

Input:

a {
  b {
    margin-top: 2rem;
  }
}

Output:

a {
    margin-top: 32px;
  b {
    margin-top: 2rem;
  }
}

/cc @MoOx because this plugin is in cssnext

@ai
Copy link
Contributor Author

ai commented Jul 9, 2015

Origin reported by @meritt postcss/postcss-nested#25

@ai
Copy link
Contributor Author

ai commented Jul 9, 2015

The solution is very easy. In pixrem you use rule.eachDecl inside css.eachRule. But eachDecl is recursive and does to rule children too. Soo you need just replace eachDecl to each with child.type == 'decl' check.

@MoOx
Copy link
Contributor

MoOx commented Jul 9, 2015

Or you can just run this plugin after the other ?
That seems a better idea.

I think some plugins (like pixrem) should be executed at the end, while some others (nested, mixins, etc) make sense at the beginning.

@ai
Copy link
Contributor Author

ai commented Jul 9, 2015

@MoOx yeap, chaging order would fix it too. But here is just a issue, misunderstanding of PostCSS API. So there is not sense to keep this bug.

@robwierzbowski
Copy link
Owner

I think this plugin should as a rule be run after Sass or any other CSS precompiler, which would avoid this bug. pixrem runs on CSS, not any number of preprocessing languages; when nesting is added to CSS then this fix would be a good one.

Does the documentation say to run preprocessors first? If not, let's add it.

@robwierzbowski
Copy link
Owner

My main concern with adding preprocessor specific code for this particular plugin is the maintainer may end up having to support all preprocessor weirdness. Variables, different syntax, etc. Better to make a rule that it only works on CSS, IMO.

@ai
Copy link
Contributor Author

ai commented Jul 9, 2015

@robwierzbowski it is not a question of plugin order. Plugin code has a bug. We got some effects with nested rules. We will got some effects in future with other cases.

Problem is only because algorithm is wrong.

@ai
Copy link
Contributor Author

ai commented Jul 9, 2015

Wrong code:

css.eachRule( (rule) => {
  rule.eachDecl( (decl) => {
    rule.append(decl.clone()) // it is a wrong code, because eachDecl is recursive
                              // and not every decl has rule as parent
  })
})

@ai
Copy link
Contributor Author

ai commented Jul 9, 2015

Right code:

css.eachRule( (rule) => {
  rule.each( (decl) => {
    if ( decl.type != 'decl' ) return;
    rule.append(decl.clone()) // it is a right code, because decl will be always a direct children of rule
  })
})

@MoOx
Copy link
Contributor

MoOx commented Jul 9, 2015 via email

@iamvdo iamvdo closed this as completed in 9aa8c88 Jul 9, 2015
@iamvdo
Copy link
Collaborator

iamvdo commented Jul 9, 2015

I'm agree with both of you. :)
This plugin should only run on CSS, but there was a bug. Now fixed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants