Static site generator using JavaScript Promises

Question

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.

I call my promises in SSG.start();

const yaml = require('js-yaml');
const fs = require('fs-extra');
const ejs = require('ejs');
const md = require('markdown-it')({
 html: true,
 linkify: true,
 typographer: true
});
let SSG = {
 outputFolder: '../output/',
 contentFolder: '../content/',
 assetsFolder: '../assets/',
 contentFiles: async function (dir, filelist) {
 return new Promise((resolve) => {
 files = fs.readdirSync(dir);
 filelist = filelist || [];
 files.forEach((file) => {
 if (fs.statSync(dir + '/' + file).isDirectory()) {
 return resolve(contentFiles(dir + '/' + file, filelist));
 } else {
 filelist.push(file);
 return resolve(filelist);
 }
 });
 })
 },
 generateHTMLFile: async function (file) {
 return new Promise((resolve) => {
 let fstream = fs.readFileSync(file, 'utf8');
 let params = fstream.match(/\+Params\+([\s\S]*?)\+\+\+/g);
 let content = fstream.match(/(\+Content[0-9]*\+|\+Content\+)([\s\S]*?)*(\+\+\+)/g);
 let contents = [];
 params = params[0].replace('+Params+', '').replace('+++', '');
 content = content[0].split('+++');
 try {
 params = yaml.safeLoad(params, 'utf8');
 for (it of content) {
 it = it.replace('+++', '').replace(/(\+Content[0-9]*\+|\+Content\+)/g, '');
 contents.push(md.render(it));
 }
 const funs = require('./functions.js');
 ejs.renderFile('../templates/' + params.template + '.ejs', {funs: funs, params: params, content: contents}, {}, (err, html) => {
 if (err) console.log(err);
 resolve({html: html, url: params.url});
 });
 } catch (e) {
 console.log(e);
 }
 });
 },
 saveHTML: function (html, url) {
 if (!fs.existsSync(this.outputFolder)) {
 fs.mkdir(this.outputFolder, { recursive: true }, (err) => {
 if (err) throw err;
 });
 }
 fs.writeFile(this.outputFolder + url + '.html', html, (err) => {
 if (err) return console.log(err);
 });
 },
 saveStaticAssets: function () {
 fs.copySync(this.assetsFolder, this.outputFolder);
 },
 start: async function () {
 try {
 let time = Date.now();
 console.log('Génération des fichiers HTML...');
 let files = await this.contentFiles(this.contentFolder);
 for (file of files) {
 await this.generateHTMLFile(this.contentFolder + file).then(({html, url}) => {
 this.saveHTML(html, url);
 });
 }
 this.saveStaticAssets();
 console.log('Généré en ' + (Date.now() - time) + 'ms');
 } catch (e) {
 console.error(e.message);
 }
 }
};
(async () => {
 await SSG.start();
})();

What do you think about it ?

• Malachi: 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!

Answer 1

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:

class SGG {
 constructor(folders = {}) {
 this.outputFolder = folders.outputFolder || '../output/'
 this.contentFolder = folders.contentFolder || '../content/'
 this.assetsFolder = folders.assetsFolder || '../assets/'
 this.templates = folders.templates || '../templates/'
 }
}

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:

const yaml = require('js-yaml');
const fs = require('fs-extra');
const ejs = require('ejs');
const md = require('markdown-it')({
 html: true,
 linkify: true,
 typographer: true,
});
const funs = require('./functions.js');
class SSG {
 constructor(folders = {}) {
 this.outputFolder = folders.output || '../output/'
 this.contentFolder = folders.contentFolder || '../content/'
 this.assetsFolder = folders.assetsFolder || '../assets/'
 this.templateFolder = folders.templateFolder || '../templates/'
 }
 contentFiles(dir, filelist) {
 const files = fs.readdirSync(dir)
 filelist = filelist || []
 for (const file of files) {
 if (fs.statSync(dir + '/' + file).isDirectory()) {
 contentFiles(dir + '/' + file, filelist)
 } else {
 filelist.push(file)
 }
 }
 return filelist
 }
 async generateHTMLFile(file) {
 const fstream = fs.readFileSync(file, 'utf8');
 let params = fstream.match(/\+Params\+([\s\S]*?)\+\+\+/g);
 let content = fstream.match(/(\+Content[0-9]*\+|\+Content\+)([\s\S]*?)*(\+\+\+)/g);
 let contents = [];
 params = params[0].replace('+Params+', '').replace('+++', '');
 content = content[0].split('+++');
 try {
 params = yaml.safeLoad(params, 'utf8');
 for (let it of content) {
 it = it.replace('+++', '').replace(/(\+Content[0-9]*\+|\+Content\+)/g, '');
 contents.push(md.render(it));
 }
 const html = await ejs.renderFile(this.templateFolder + params.template + '.ejs', {
 funs: funs,
 params: params,
 content: contents,
 }, {})
 return {
 html, 
 url: params.url,
 }
 } catch (e) {
 console.log(e);
 }
 }
 async saveHTML(html, url) {
 if (!fs.existsSync(this.outputFolder)) {
 await fs.mkdirSync(this.outputFolder, {recursive: true});
 }
 await fs.writeFileSync(this.outputFolder + url + '.html', html);
 }
 saveStaticAssets() {
 fs.copySync(this.assetsFolder, this.outputFolder);
 }
 async start() {
 try {
 let time = Date.now();
 console.log('Génération des fichiers HTML...');
 const files = this.contentFiles(this.contentFolder);
 for (const file of files) {
 await this.generateHTMLFile(this.contentFolder + file).then(({html, url}) => {
 this.saveHTML(html, url);
 });
 }
 this.saveStaticAssets();
 console.log('Généré en ' + (Date.now() - time) + 'ms');
 } catch (e) {
 console.error(e.message);
 }
 }
};
(async () => {
 const ssg = new SSG()
 await ssg.start();
})();

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:

await this.generateHTMLFile(this.contentFolder + file).then(({html, url}) => {
 this.saveHTML(html, url);
});`

It's maybe just me, but I don't like this mixture of await and then. I'd just suggest that if you're going with await, keep it that way. My suggestion here would be to go with something like this:

const {html, url} = await this.generateHTMLFile(this.contentFolder + file)
await this.saveHTML(html, url)

I've also just noticed that you can optimize the solution for speed by awaiting on multiple promises:

await Promise.all(files.map(async (file) => {
 const {html, url} = await this.generateHTMLFile(this.contentFolder + file)
 await this.saveHTML(html, url)
}))

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.

• Thomas: 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 ?
• Thomas: 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
• Pritilender: @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]
• Pritilender: 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.

Answer 2

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

SSG.start();

Use modern syntax

• When adding functions to an object use the function shorthand saveStaticAssets: function () { ... }, becomes saveStaticAssets() { ... },

• Use object property shorthand if you have the variable names defined. eg {funs: funs, params: params , content: content} becomes {funs, params, content}

• Use parameter defaults eg contentFiles: async function (dir, filelist) { /*...*/ filelist = filelist || []; becomes async contentFiles(dir, filelist = []) {

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)

.

const yaml = require('js-yaml');
const fs = require('fs-extra');
const ejs = require('ejs');
const funs = require('./functions.js');
const md = require('markdown-it')({html: true, linkify: true, typographer: true});
const FOLDERS = {output: '../output/', content: '../content/', assets: '../assets/'};
const REGS = Object.freeze({
 match: {
 constent: /(\+Content[0-9]*\+|\+Content\+)([\s\S]*?)*(\+\+\+)/g,
 param: /\+Params\+([\s\S]*?)\+\+\+/g
 },
 replace: {content: /(\+Content[0-9]*\+|\+Content\+)/g}
});
start().catch(e => console.error(e));
async function contentFiles(directory, filelist = []) {
 const files = await fs.readdir(directory);
 return (await Promise.all(files.map(async file => 
 (await fs.stat(directory+ '/' + file)).isDirectory() ? 
 contentFiles(directory+ '/' + file, filelist) : 
 file
 )));
}
async function generateHTMLFile(file) {
 const contents = [], fstream = await fs.readFile(file, 'utf8');
 const content = fstream.match(REGS.match.content)[0].split('+++');
 var params = fstream.match(REGS.match.params);
 params = yaml.safeLoad(
 params[0].replace('+Params+', '').replace('+++', ''), 'utf8'
 );
 for (item of content) {
 contents.push(md.render(
 item.replace('+++', '').replace(REGS.replace.content, '')
 ));
 }
 return ejs.renderFile(
 '../templates/' + params.template + '.ejs', {funs, params, content}, {}
 );
}
async function saveHTML({html, url}) {
 const createDir = await fs.exists(FOLDERS.output);
 if (createDir) { await fs.mkdir(FOLDERS.output, {recursive: true}) }
 await fs.writeFile(FOLDERS.output + url + '.html', html);
}
async function start() {
 await Promise.all(
 (await contentFiles(FOLDERS.content)).map(async file =>
 saveHTML(await generateHTMLFile(FOLDERS.content + file))
 )
 );
 fs.copy(FOLDERS.assets, FOLDERS.output);
 // return fs.copy(FOLDERS.assets, FOLDERS.output); // maybe return promise
}

• Thomas: Thank you for your review, I didn't knew the existence of Object.freeze() and some modern syntaxes, I have truly learned a lot.
• Thomas: 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.
• Blindman67: @Thomas Yes indeed that is what your code does, I will update the example.