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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix to issue with async output and missing output directory #27

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

ostowe
Copy link
Collaborator

@ostowe ostowe commented Jun 12, 2018

Sorry for the huge PR. I also switched all the tests to jest, so this should address #14 as well.

More detail about what I'm fixing here: for a project using webpack in which most CSS imports are happening in JS files, postcss will run once for each import. Due to an improperly handled call to an async fs function (I can't remember which one), this was causing only part of each chunk of critical CSS to get output. To solve this I switch to using fs-extra (which promisifies all fs functions, among other things) and switched to using async/await.

LMK what you want to do with this PR, I'm happy to use my fork until then 馃帀

@ostowe
Copy link
Collaborator Author

ostowe commented Jun 12, 2018

Alrighty, after testing a bit more I guess this doesn't actually fix my issue. Seems like unless we can figure out some way to open a writable stream, keep it open while webpack is running, then close the stream when it鈥檚 done I don鈥檛 think we鈥檒l be able to figure this one out. Bummer.

You're welcome to cherry pick the testing updates, though.

@zgreen
Copy link
Owner

zgreen commented Jun 13, 2018

I'd love to get this merged, if only for the testing updates alone. Very rad. Thanks!

Possible to create a failing test case for the use case you describe?

- Convert some reamaining promise logic to async/await
- Add ignoreSelectors option and functionality
@ostowe
Copy link
Collaborator Author

ostowe commented Jun 13, 2018

@zgreen sweet! I actually managed to fix this, though it's probably a little janky. I added bottleneck to rate-limit file writes, preventing the overlap issue.

I'll also see if I can come up with a test for this.

@ostowe
Copy link
Collaborator Author

ostowe commented Jun 19, 2018

@zgreen ready for a review when you get a chance (assuming you want to code review it)

@zgreen
Copy link
Owner

zgreen commented Jun 25, 2018

Thanks @ostowe. Was out all last week but will look this week :)

@zgreen
Copy link
Owner

zgreen commented Jul 12, 2018

Sorry for my delay here... will definitely look soon.

@zgreen
Copy link
Owner

zgreen commented Sep 23, 2018

This has lingered too long :/ Still want to get it merged.

My plan is to cherry-pick the jest work so we can get that merged first, and then address the issue raised here, as my sense is that there's more to review/discuss there. Thanks again :)

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