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

Large preprocessor outputs hit the maxBuffer limit #87

Merged
merged 5 commits into from Jul 13, 2013

Conversation

freejosh
Copy link
Contributor

@freejosh freejosh commented Jul 5, 2013

When using one main file that imports all my CSS with a preprocessor I was hitting the maxBuffer limit. I've changed exec to spawn so that the output gets built from the stream, without a limit.

Also fixed a typo in the regex that checks for Sass partials.

@marrs
Copy link
Collaborator

marrs commented Jul 5, 2013

Thanks for the request. Can you provide a regression test for child_process.spawn and resubmit?

@freejosh
Copy link
Contributor Author

freejosh commented Jul 6, 2013

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?

@marrs
Copy link
Collaborator

marrs commented Jul 8, 2013

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.

@freejosh
Copy link
Contributor Author

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.

@marrs
Copy link
Collaborator

marrs commented Jul 12, 2013

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.

@freejosh
Copy link
Contributor Author

Sounds good. Yea the warning should be something like:

exec, by default, only returns 200k of data. Use spawn if you're expecting larger return data.

marrs added a commit that referenced this pull request Jul 13, 2013
Large preprocessor outputs hit the maxBuffer limit
@marrs marrs merged commit a2235d5 into jacobrask:master Jul 13, 2013
@marrs
Copy link
Collaborator

marrs commented Jul 13, 2013

Thanks again for the patch. It'll be in the next maintenance release

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