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

Replace Once with OnceExit to do the compression last #20

Merged
merged 4 commits into from Apr 5, 2021

Conversation

onigoetz
Copy link
Contributor

@onigoetz onigoetz commented Feb 7, 2021

Hi,

With the recent update of postcss-nested to use RuleExit (https://github.com/postcss/postcss-nested/blob/main/index.js#L135) it seems that Once that postcss-csso is fired too early.

Making postcss-csso run before other postcss plugins finish converting the CSS file and thus can't parse the currently generated CSS.

Using OnceExit instead of Once moves this processing when everything else is finished.

I changed a test as there was a test failure

  1. edge cases
    should process partial inited nodes:
    TypeError: Cannot read property 'toLowerCase' of undefined
    at getEvents (node_modules/postcss/lib/lazy-result.js:29:21)
    at toStack (node_modules/postcss/lib/lazy-result.js:56:14)

The line of the error is : https://github.com/postcss/postcss/blob/main/lib/lazy-result.js#L52

Which means that there is no way for postcss-csso to handle a partially inited declaration node because postcss can't.

This is why I changed the test to only test for an unfinished @ rule since that actually works.

@lahmatiy
Copy link
Owner

lahmatiy commented Apr 5, 2021

I added a test for postcss-nested and revert changes in "should process partial inited nodes" test with a fix of it.
Looks like PR solves the issue #19.
@onigoetz Thank you very much for the fix and clarifications 🙏

@lahmatiy lahmatiy merged commit e6d81c8 into lahmatiy:master Apr 5, 2021
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.

None yet

2 participants