Skip to content

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

Open
ostowe wants to merge 8 commits into
zgreen:masterfrom
ostowe:output-fix
Open

Fix to issue with async output and missing output directory#27
ostowe wants to merge 8 commits into
zgreen:masterfrom
ostowe:output-fix

Conversation

@ostowe

@ostowe ostowe commented Jun 12, 2018

Copy link
Copy Markdown
Collaborator

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

ostowe commented Jun 12, 2018

Copy link
Copy Markdown
Collaborator Author

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’s done I don’t think we’ll be able to figure this one out. Bummer.

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

@zgreen

zgreen commented Jun 13, 2018

Copy link
Copy Markdown
Owner

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

ostowe commented Jun 13, 2018

Copy link
Copy Markdown
Collaborator Author

@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

ostowe commented Jun 19, 2018

Copy link
Copy Markdown
Collaborator Author

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

@zgreen

zgreen commented Jun 25, 2018

Copy link
Copy Markdown
Owner

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

@zgreen

zgreen commented Jul 12, 2018

Copy link
Copy Markdown
Owner

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

@zgreen

zgreen commented Sep 23, 2018

Copy link
Copy Markdown
Owner

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.

2 participants