search

dwyl / sendemail

💌 Simplifies reliably sending emails from your node.js apps using AWS Simple Email Service (SES)
182 stars 26 forks source link

What is the reason behind of using readFileSync vs readFile? #74

Open heron2014 opened 7 years ago

heron2014 commented 7 years ago

Related to the following line: https://github.com/dwyl/sendemail/blob/master/lib/index.js#L38

I was wondering if you guys could clarify the above line for me.

Aren't we blocking the thread by using sync method in node? What happen if there is an error of reading the file?

Thanks!

nelsonic commented 7 years ago

Hi @heron2014 hope you are well and thank you for asking this question. Did this cause you grief while using the module? (i.e. did it cause you to lose time debugging because a template did not exist or would not compile...?) 🤔

The reason for opening/reading the file synchronously on /lib/index.js#L38 is mentioned in the JSDoc comment on lib/index.js#L28 but I can see how/why it's unclear ... Yes, it's "blocking" however opening of the template will only be performed once as it immediately gets cached as for error-handling if the template does not exist we want to throw (or bubble up) an error because we want to notify the application writer that the template does not exist. We could write more code here to do this anychronously but it would add more complexity to the module while only saving a mili-second each time the app is started. Synchronously opening a file once per server-start will not affect performance for the end-user it only affects the server startup time. ⌛️

Rather than writing branching code to handle the error in our own custom way, we let Node.js report it's "core" error to the developer e.g. ENOENT which will tell the Developer exactly what line the error is on and (hopefully) they can figure out how to fix it... 🔍

heron2014 commented 7 years ago

Thanks @nelsonic for detailed explanation!

I guess the line https://github.com/dwyl/sendemail/blob/master/lib/index.js#L39 could be moved to if block (https://github.com/dwyl/sendemail/blob/master/lib/index.js#L42) because we want the compiling only to happen when we need it (if the file is not cached). So the line (https://github.com/dwyl/sendemail/blob/master/lib/index.js#L39) doesn't need to run each time. Am I thinking right in here?

Thanks again.

nelsonic commented 7 years ago

@heron2014 yes /lib/index.js#L39 should be moved to the if statement/block on /lib/index.js#L42-L44 to avoid unnecessary computation (re-compiling templates). Please Pull Request the Update and assign it to @shouston3 thanks. 👍