Closed freejosh closed 11 years ago
Thanks for the request. Can you provide a regression test for child_process.spawn and resubmit?
I'm not too sure how to do that, do I make a file like test/urlRelative.js
? If that's what you mean, would you say it's fine to skip passing in an actual preprocessor command, and just run cat
to read the contents of a large CSS file?
Yes, exactly. The urlRelative test is written using Mocha and Chai. The others are written in NodeUnit. You can use whichever you find more comfortable.
If reading the contents of a large file directly still reproduces the error that you fixed then I would say it's preferable to skip the pre-processor command. Ideally we want the fewest/simplest conditions to reproduce the error.
Let me know if adding the tests here was fine, or if they should be in a separate pull request, to test with unfixed code first.
Here is fine. I trust that you watched the test fail and then made it pass. :)
Although, now that I understand the nature of the bug, I'm wondering if it's worth including this test. I'm not sure I want to add a 200M stub to the repo just to reproduce, but without it the test is at best useless.
Let's merge the branch without the test. I think a better idea for the future is to hook a linter into the test suite to check the code for child_process.exec
and warn the user against using it.
Sounds good. Yea the warning should be something like:
exec
, by default, only returns 200k of data. Usespawn
if you're expecting larger return data.
Thanks again for the patch. It'll be in the next maintenance release
When using one main file that imports all my CSS with a preprocessor I was hitting the maxBuffer limit. I've changed
exec
tospawn
so that the output gets built from the stream, without a limit.Also fixed a typo in the regex that checks for Sass partials.