search

jsreport / jsreport-wkhtmltopdf

jsreport recipe for rendering pdf using wkhtmltopdf
http://jsreport.net
MIT License
8 stars 9 forks source link

added option maxBuffer for the reporter node #9

Closed MI53RE closed 7 years ago

MI53RE commented 7 years ago

That can be setted in dev/prod.config.js files such as 

{
  ...
  "reporter": {
    "maxBuffer": 512000 // will allow a buffer size of 512Mb
  }
  ...
}
pofider commented 7 years ago

Could you rather scope the config only to the wkhtmltopdf node?

this.maxBuffer = definition.options.hasOwnProperty('maxBuffer') ? definition.options.maxBuffer : (1000 * 1024)

It would be set afterwards like this

{
   ...
   "wkhtmltopdf": {
     "maxBuffer": xxxx
   }
}

Thank you

MI53RE commented 7 years ago

Okay, done (and tested localy) as per your request =) (just need to wait for Travis confirmation!)

I did set it in the reporter though as I believed (and still do) that this should be a global parameter and not only proper to wkhtmltopdf recipe, as this parameter has impact on the node child_process

what do you think about it? =)

pofider commented 7 years ago

Thank you.

I think that every child process run has different expectations. So it should be rather scoped option. Like here we for example expect small stdout, because it just logs, but here we expect giant stdout https://github.com/pofider/html-to-xlsx/blob/master/lib/dedicatedProcessStrategy.js#L26 However with reasonable defaults, there is no need to even think about it.

If we come out about this otherwise in the future, I'll gladly introduce new global maxBuffer option, but the first step is to have it scoped I think.

pofider commented 7 years ago

Thank you for your contribution!

MI53RE commented 7 years ago

You're welcome! Thank you for accepting the feature!

I do understand better your point after looking into this example. Indeed default would not be the same value regarding the recipe used.

Just asking but: regarding that "eventual" maxBuffer global in the future, would it be easier/simpler to just add a execOptions node so users can set any options accepted by child_process without having the need to adapt the code again? (of course I keep the "eventualy" strongly in mind!!)

pofider commented 7 years ago

You are right. The execOptions makes sense. Someone could want to send also env to the child for example. We already do the similar think here https://github.com/pofider/node-script-manager/blob/master/lib/manager-processes.js#L19

I'll changed that before publishing. I guess tomorrow. Thanks for the suggestion.

pofider commented 7 years ago

I apologize for delay here. It somehow got out of my sight.

Right now this is released in 1.4.0.

Thank you for your contribution here.