3
\$\begingroup\$

This is my first attempt to a serious Node.js application, I've tried to follow the async style of Node at my best, but I have several doubts and I am sure that this code can be massively improved.

The idea behind this little service is to get a minified stacktrace and return a 'translated' stacktrace, if there is any error/exception the code should just log it and return to who request the minified version submitted in the first instance.

I broke up the code in order to add my concerns, you can find the full source here

I've followed a core -> 3rd party -> my_source order for the node request, is that the most common standard?

var util = require('util'),
    https = require('https'),
    http = require('http'),
    async = require('async'),
    sourcemaps = require('source-map'),
    cache = require('./limitedItemsCache')(50),
    logger = require('./winstonConfig');

This is the only function I export, I've used module.exports as I find it more consistent, is that correct?

module.exports.translateStacktrace = function (errorObjectRaw, callback) {
    var errorObject;

Here I had to capture eventual syntax errors with the try..catch block, is this the best way to operate in this situation? In the rest of the code I followed the callback fn (err, result) paradigm

    try {
        errorObject = JSON.parse(errorObjectRaw);
    } catch (err) {
        logger.error(err);
        callback(null, errorObjectRaw);
    }

    var lines = errorObject.stackTrace,
        firstLineUrl = (lines[0].match(/http.*js/)[0]),
        firstLineSegments = firstLineUrl.split('/'),
        decodedLines = {};

    // if the error starts in the vendor file we're not interested in a translation

Is there a better way to handle early returns than adding an if..else control block? I find it kinda weird, but I could not find any other better solution

    if (firstLineUrl.indexOf('vendor') !== -1) {
        logger.info('stacktrace starting in vendor');
        callback(errorObjectRaw);
    } else {
        decodedLines.fileName = firstLineSegments.splice(firstLineSegments.length - 1, 1)[0];
        decodedLines.baseUrl = firstLineSegments.join('/');
        decodedLines.mapUrl = util.format('%s/%s.map', decodedLines.baseUrl, decodedLines.fileName);
        decodedLines.positions = [];

        async.parallel([
            async.apply(_decodeSingleLines, lines, decodedLines),
            async.apply(_getSourceMap, decodedLines.mapUrl, decodedLines.fileName)
        ], function (err, results) {

Again, for errors, is this the best options when using the callback error paradigm? Am I force to use if.. else blocks?

            if (err) { 
                callback(err);
            } else {
                _translatePositions(results[0], results[1], errorObject, callback);
            }
        });
    }
}

function _decodeSingleLines (lines, decodedLines, callback) {
    for (var i = 0, ii = lines.length; i < ii; i++) {
        var segments = lines[i].replace('"', '').split('/'),
            minifiedVariable = segments[0].split('@')[0],
            rawFileInfo = /(.+):(\d+):(\d+)/.exec(segments[segments.length - 1]);

        decodedLines.positions.push({
            source: rawFileInfo[1],
            line: parseInt(rawFileInfo[2]),
            column: parseInt(rawFileInfo[3]),
            name: minifiedVariable
        });
    }
    callback(null, decodedLines);
}

function _getSourceMap (url, fileName, callback) {
    // preprod has self signed weird certificate
    process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0";

    var cached = cache.get(fileName);
    if (cached) {
        callback(null, cached.element);
    } else {
        var protocol = url.indexOf('https://') > -1 ? https : http;
        protocol.get(url).on('response', function (response) {
            var rawSourceMap = '';
            response.on('data', function (chunk) { 
                rawSourceMap += chunk; 
            });

            response.on('end', function () {
                var consumer = new sourcemaps.SourceMapConsumer(rawSourceMap);
                cache.put(fileName, consumer);
                callback(null, consumer); 
            });
        });
    }
}

Here I had to pass the endcallback to this function in order to make the code more readable, is this bad?

function _translatePositions (decodedLines, consumer, errorObject, endCallback) {
    var transformIterator = {
        consumer: consumer,
        fileName: decodedLines.fileName,
        getOriginalPositions: _getOriginalPositions,
    };

    async.map(
        decodedLines.positions, 
        transformIterator.getOriginalPositions.bind(transformIterator),
        function (err, result) {
            if (err) {
                endCallback(err);
            } else {
                errorObject.stackTrace = result;
                endCallback(null, errorObject);
            }
        }
    );
}

function _getOriginalPositions (transformedPosition, callback) {
    var originalPosition = transformedPosition,
        bundleName,
        stacktraceString;

    if (transformedPosition.source.indexOf('vendor') === -1) {  
        originalPosition = this.consumer.originalPositionFor({ 
            line: transformedPosition.line, 
            column: transformedPosition.column 
        });
        bundleName = '|' + this.fileName + '|';
        originalPosition.source = 
            originalPosition.source
                .replace(/\\/g, '/')
                .replace(/\.\.\/client\/js\/src\/(desktop|mobile)\//, '');
    }

    stacktraceString = util.format(
        '%s@%s|L%s C%s%s',
        originalPosition.name,
        originalPosition.source,
        originalPosition.line,
        originalPosition.column,
        bundleName || ''
    );

    callback(null, stacktraceString);
}

Additional questions:

  • Should I break the code in more modules?
  • Is the way I ordered the code understandable or confusing?

Any observation is truly appreciated. I am really trying to write decent Node.js code but the aync nature of node and the confusing (for me) error handling is not making this an easy job.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.