I have created a small static site generator (for learning purposes) in Javascript ES6, using Promises, but I am not certain of how to use them well. The code below works fine, but I wonder if i can't write my promises a better way.

\$\begingroup\$Welcome to Code Review! Thanks for this good question - I hope you get some good reviews, and I hope to see more of your contributions here in future!\$\endgroup\$
– Malachi♦May 27 at 2:09

2 Answers
2

First thing that bother me is why let SSG? Why is it an object? If you're using ES6+ (as you do), I'd go with making this a class. It's more obvious and more readable. Note that while doing this, you'll have to alter the function signature, i.e. generateHtmlFile: async function (file) { ... } becomes async generateHtmlFile(file) { ... }.

Since you have folder names stored as a properties of your object (outputFolder, contentFolder and assetsFolder), they should be moved now to the constructor. There is also a good candidate for another new property on line 51: ../templates/ should be also extracted. This is because there are no properties in JS class ATM. Also, while we're on the topic of class properties, I'd suggest that you should pass an object with folder names as a constructor argument. That way you can easily change the folders you're using by just instantiating an object with another folder names. When we sum up the info about properties, here's what it should look like:

The second thing that I didn't notice when I looked at the code here is that you have a some implicit variables: files on line 19 and it on line 45. So just add const in front of those two and you'll be fine.

At line 49, there is a require. I'd move it to the top of the file because that way you know right on what are the dependencies of your file. And it's more cleaner code without some unexpected turns.

Now, to the methods. First one is contentFiles which is just a recursive call to find out all the files in a contentFolder directory. I see that you are probably confused with promises because you use async and new Promise in the same function. When making function an async, you should know that whatever you return from the function, it will be wrapped inside a promise and it also gives you the ability to use await inside it. But since you don't have await here, then there is no need for making this function an async one. You are already returning a promise. But, I think that you don't need a promise at all! You are using these fs-extra methods that are not returning a promise nor have a callback, so they are perfectly synchronous functions. Also, the .forEach got all of this tangled a bit, so if you remove it and use for..of, you can untangle yourself and get a cleaner code.

The next method, generateHTMLFile has similar issue with promises as the one above. From ejs' github, I noticed that if you don't pass the callback function, it will return a promise. So what should be done here is to remove new Promise... thing and keep async and rewrite the ejs.renderFiles not to use the callback function.

The method saveHTML is confusing a bit: you're using sync methods everywhere, but here you decided to go with mkdir instead of mkdirSync or writeFileSync.

Once you apply all of what I've said, you should get something like this:

Last note that I'd like to make is that Node already has a pretty decent file system utilities. The only thing that I've noticed that is different from fs-extra is that sync functions do not return promises, but they rather act as a regular functions. Anyhow, I'd argue that you don't need fs-extra and use fs instead. But it's up to you.

I maybe missed something or maybe got into details and nitpicking more than needed, but this is a basic refactor that I would do to your code. I hope that I didn't break some things while doing this :)

Edit:
While looking at the code for the second time, I noticed that I've missed this line in start:

This way you start multiple promises in parallel (again, not a true parallelization since JS is single threaded) and await on all of them to finish before moving on. This way you'll get a bit faster execution. Note that if one promise throws and error, this line will also throw an error before waiting on the other promises to end.

\$\begingroup\$Thanks for your review, i will adapt some things in my code. I have some questions and remarks. Why and how contentFiles() is not async or returning a Promise, because when I call it on start() I tell to await the method ? I have tested your method with and without async and both seems to work well, how can I be sure without a Promise or async that, in start(), files would get all files before iterate it in the for .. of .. below ?\$\endgroup\$
– ThomasMay 27 at 12:56

\$\begingroup\$Also I disagree with the removal of the Promise in generateHTMLFile(), I want my Promise return html & url, what I did in my code, but in yours I can't use the Promise of the EJS method for the next function I call : saveHTML(). I would let that part as is in my code. FYI, I use fs-extra (which implements all fs methods) to use the copySync() method, I was a bit lazy about creating myself a function which does the same as copySync() =) Thanks again for reviewing my code\$\endgroup\$
– ThomasMay 27 at 13:00

\$\begingroup\$@Thomas I missed to change await contentFiles() to contentFiles() in start. To answer your question from the first comment: you know this by looking inside contentFiles method. You are not calling any function that returns a promise there, so you can be sure that whatever contentFiles returns, it won't be a promise. Also, you're not stating that the function is async. All the methods you are using there are synchronous and the whole iteration will be done just as you imagine it: one by one. It won't run in parallel. [Contd. in next comment]\$\endgroup\$
– PritilenderMay 27 at 14:18

\$\begingroup\$One more thing: promise doesn't mean parallel execution. Yes, you can have few promises running in parallel (not truly parallel, but don't want to get into much JS details), but if you are looping over an array with for..of and have await inside it, it'll be done a sequential order. Regarding the second comment and return value of generateHTMLFile, I'll update the code to address that also.\$\endgroup\$
– PritilenderMay 27 at 14:21

Review

Your code is a mess and breaks about every async rule in the book.

JavaScript is slow but because Node.js is event driven it can still deliver a very efficient server solution. However if you start using synchronous IO calls you negate any benefit that node.js provides and in effect turn a high throughput server into a slow dinosaur.

General advice is DON'T use synchronous calls if there is an asynchronous alternative.

???

What are you waiting to do? in...

(async () => {
await SSG.start();
})();

You dont do anything after the function so in effect you have just made a complex version of

No need for Object SSG

You have created a module that contains a static single instance of SSG. There is no benefit gained by creating the object. All it does to force you to use a more verbose syntax when accessing references eg this.foo can be simply foo

Try Catch?

Promises/Async come with error handling built in. You should not be using try, catch blocks in async and promise based code.

Careful console may block

The global console can be synchronous and thus is a blocking IO operation (blocks events) that flies in the face of the ideal of node.js being a non blocking event driven server.

Avoid logging to the console for all but the most important information tracking (even if not blocking console noise is bad)

Rewrite

The rewrite assumes you only want the single static instance run once.

This rewrite is untested, somewhat of a guess as to your needs and is purely as example only It is not intended as a working alternative.

Single error handler so stops on any error!!!

All functions are async

Removed the redundant SSG wrapper on functions and properties.

Removed all synchronous fs-extra calls in favor of async versions.

Using await to idle execution when ever possible.

Uses Promise.all to handle async lists/arrays

Moved folder names to Object FOLDERS

Created REGS to hold regExp that where mixed with code. It is frozen to prevent mutation.

Moved const funs = require('./functions.js'); to local global scope.

Note that this will exit before the final fs.copy is complete. If there is an error it will not be caught. Un-comment line below to await proper end.

start and contentFiles may not work together at best possible performance (or at all) (I am assuming there is no dependency between files in start an the result of contentFiles)

\$\begingroup\$Thank you for your review, I didn't knew the existence of Object.freeze() and some modern syntaxes, I have truly learned a lot.\$\endgroup\$
– ThomasMay 27 at 15:21

\$\begingroup\$Also I just wanted to point out including the function writeFile(); on a ternary operator on saveHTML(); is wrong, i want to write a file in either ways.\$\endgroup\$
– ThomasMay 27 at 15:58

\$\begingroup\$@Thomas Yes indeed that is what your code does, I will update the example.\$\endgroup\$
– Blindman67May 27 at 17:32