Skip to content

Conversation

@ludofischer
Copy link
Collaborator

I've noticed cssnano/cssnano#954 assumes there's a version of this with the PostCSS 8 API, but I could not find it either a release, pull request or branch, so here it it.

I've used the visitor API thinking it should be safe from infinite loops, as once the calc() declarations are gone, the transform() function will exit immediately.

@Semigradsky
Copy link
Member

.travis.yml should be updated.

src/index.js Outdated
Declaration(node, {result}) {
transform(node, "value", options, result);
},
AtRule(node, {result}) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

hm, maybe we should remove this from major release? What is use case for at-rule(s)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since there is a mediaQueries option, I think the use case for at-rules is calc() in media queries, like:

@media (max-width: calc(40em * 2)) { ... }

Copy link
Collaborator

Choose a reason for hiding this comment

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

hm, it sounds reasonable, I think we should enable it by default

@ludofischer
Copy link
Collaborator Author

I would like to add an integration test with other plugins on realistic CSS, to check that processing terminates without errors. But I am unsure what plugins and sample CSS to pick. Maybe autoprefixer?

@ludofischer
Copy link
Collaborator Author

I think this pull request should not be merged in this state, there's a chance of infinite loops apprearing. I've added a test that produces an infinite loop: adding a plugin that changes whitespace inside the calc() property. I guess the 'right' solution would be to skip writing the value if we detect the parsed value AST are equivalent? Or just use Once() :-)?

@alexander-akait
Copy link
Collaborator

@ludofischer Let's use Once

* BREAKING CHANGE: require PostCSS 8 and Node >= 10..
* Move postcss to peer dependencies.
@alexander-akait
Copy link
Collaborator

@Semigradsky Can you help and do major release due cssnano/cssnano#975 (comment)?

@Semigradsky Semigradsky merged commit 6826ba5 into postcss:master Jan 3, 2021
@Semigradsky
Copy link
Member

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.

3 participants