Open Source Report

We have analyzed code from more than 200 open source JavaScript projects in GitHub. This report highlights results from several popular projects.

We hope to help developers improve the quality and are willing to work on improving the quality of open source project.

The results of 20 projects from GitHub are summarized below and we are going to discuss some projects in depth.

Summary

20 Projects

1,162,962 Total LoC

Name Lines of Code High/Medium
Issues
Low
Issues
adobe/brackets 69,624 17 54
ag-grid/ag-grid 24,371 9 59
angular/angular.js 118,482 3 9
Automattic/calypso 275,728 100 234
chartjs/Chart.js 6,819 7 6
codemirror/CodeMirror 46,009 20 158
facebook/react 69,396 129 223
jquery/jquery-mobile 12,092 5 17
less/less.js 10,715 9 27
lodash/lodash 5,744 3 4
Microsoft/vscode 340,208 66 288
moment/moment 42,694 11 92
mozilla/pdf.js 55,250 8 46
naver/egjs 11,490 3 39
naver/jindojs-jindo 15,952 18 193
novus/nvd3 11,369 11 63
NUKnightLab/TimelineJS 9,356 18 102
petkaantonov/bluebird 7,049 11 37

Issue Chart

Below charts show the number of top 10 violated rules detected in the above 20 projects.

High and Medium Impacts (Top 10)

Low Impacts (Top 10)


We posted an article about common pitfalls based on this data. You can see some violated patterns with real world examples in the article. Also when you want to know what each rule means, check out this documentation.

Detailed Report

We have regularly analyzed the following projects and published the result to the service.

Let's see some issues in depth. Below sections have the following format:

  • Grade badge: When you click, you can go to the dashboard to see detailed issues.
  • Issue
    • Message
    • Code fragment
    • File
    • Description

Adobe Brackets

DeepScan Grade

NULL_POINTER

Variable 'match' has a null value originated from assignment 'match = null' at line 154. But its property is accessed at this point.

src/LiveDevelopment/MultiBrowserImpl/language/HTMLInstrumentation.js

function _getMarkerAtDocumentPos(editor, pos, preferParent, markCache) {
    var marks, match;

    markCache = markCache || {};
    marks = _getSortedTagMarks(editor._codeMirror.findMarksAt(pos), markCache);
    if (!marks.length) {
        return null;
    }

    // The mark with the latest start is the innermost one.
    match = marks[marks.length - 1];
    if (preferParent) {
        // If the match is exactly at the edge of the range and preferParent is set,
        // we want to pop upwards.
        if (_posEq(match.range.from, pos) || _posEq(match.range.to, pos)) {
            if (marks.length > 1) {
                match = marks[marks.length - 2];
            } else {
                // We must be outside the root, so there's no containing tag.
                match = null;
            }
        }
    }

    return match.mark;
}

Variable match is assigned to null at line 154. So, a TypeError can be thrown in the line 159.

This pitfall is commonly found because a developer can miss the test for the boundary or exceptional value.


NULL_POINTER

Value for the first parameter 'editor' is missing here, but its property is accessed in the function body at line 317.

src/extensions/default/CodeFolding/main.js

function removeGutters(editor) {
    Editor.unregisterGutter(GUTTER_NAME);
    $(editor.getRootElement()).removeClass("folding-enabled");
    CodeMirror.defineOption("foldGutter", false, null);
}
...
function deinit() {
    ...
    // Remove gutter & revert collapsed sections in all currently open editors
    Editor.forEveryEditor(function (editor) {
        CodeMirror.commands.unfoldAll(editor._codeMirror);
    });
    removeGutters();
}

Argument editor is undefined because removeGutters is called without any arguments. By jQuery, a TypeError will not be thrown but the class will not be removed contrary to the developer's intention.

I think removeGutters should be placed under the callback of forEveryEditor like:

    // Remove gutter & revert collapsed sections in all currently open editors
    Editor.forEveryEditor(function (editor) {
        CodeMirror.commands.unfoldAll(editor._codeMirror);
        removeGutters(editor);
    });

CONSTANT_CONDITION

Condition 'self.cachedHints' is always false at this point because it conflicts with previous condition 'this.cachedHints' at line 126.

src/extensions/default/UrlCodeHints/main.js

if (this.cachedHints) {
    // use cached hints
    unfiltered = this.cachedHints.unfiltered;

} else {
    directory = FileSystem.getDirectoryForPath(targetDir);
    self = this;

    if (self.cachedHints && self.cachedHints.deferred) {
        self.cachedHints.deferred.reject();
    }

self.cachedHints at line 134 is null because it is in the else branch of the condition at line 126. So a rejection cannot be done.

I believe that this block at line 134 and 136 is useless so should be removed for maintainability.


MISSING_RETURN_VALUE

No value is returned from function 'createBody' defined at line 153.

src/LiveDevelopment/Agents/RemoteFunctions.js

createBody: function () {
    if (this.body) {
        return;
    }

    // compute the position on screen
    var offset = _screenOffset(this.element),
        x = offset.left,
        y = offset.top + this.element.offsetHeight;

    // create the container
    this.body = window.document.createElement("div");
    this.body.style.setProperty("z-index", 2147483647);
    this.body.style.setProperty("position", "absolute");
    this.body.style.setProperty("left", x + "px");
    this.body.style.setProperty("top", y + "px");
    this.body.style.setProperty("font-size", "11pt");

    // draw the background
    this.body.style.setProperty("background", "#fff");
    this.body.style.setProperty("border", "1px solid #888");
    this.body.style.setProperty("-webkit-box-shadow", "2px 2px 6px 0px #ccc");
    this.body.style.setProperty("border-radius", "6px");
    this.body.style.setProperty("padding", "6px");
},
...
show: function () {
    if (!this.body) {
        this.body = this.createBody();
    }

The function createBody does not return any value, so this.body in the show function is assigned to undefined.

Using a return value of the function which does not return should be avoided.

Automattic Calypso

DeepScan Grade

EVENT_HANDLER_INVALID_THIS

Function 'this.recordClick' is used as a React event handler without 'this' binding. But 'this' object is accessed in the function body at line 26.

client/blocks/reader-site-stream-link/index.jsx

const omitProps = [ 'feedId', 'siteId', 'post' ];

return (
    <a { ...omit( this.props, omitProps ) } href={ link } onClick={ this.recordClick }>
        <Emojify>

Since React does not provide this object when calling an event handler, a TypeError exception is thrown when you access the property of this in the event handler.

To solve this problem, you need to do one of the following:

  • Use Function.prototype.bind() to explicitly bind this with the handler
  • Use an arrow function of ES6 that automatically bind this

INSUFFICIENT_NULL_CHECK

Variable 'selectedSite' is null checked here, but its property is accessed without null check prior at line 430.

client/my-sites/checkout/checkout-thank-you/jetpack-thank-you-card.jsx

const { translate, selectedSite } = this.props;
const manageUrl = selectedSite.getRemoteManagementURL() + '§ion=plugins-setup';

if ( ! selectedSite || selectedSite.canManage() ) {
    return null;

selectedSite is null checked in the condition. It implies the prop can be null, so it should be checked. But it is accessed like selectedSite.getRemoteManagementURL() prior the sanity check.

I believe that the sanity check for selectedSite should have first place in the function.


REACT_API_TYPO

'PropTypes' could be a typo. Did you mean 'propTypes' instead?

client/components/social-icons/facebook.jsx

/**
 * External dependencies
 */
import React, { PureComponent } from 'react';
import PropTypes from 'prop-types';
import classNames from 'classnames';
import { omit } from 'lodash';

export default class FacebookIcon extends PureComponent {
    static PropTypes = {
        isDisabled: PropTypes.boolean,

React’s PropTypes allows to specify the types of properties (whether they are required, the type of values, etc.) that a component has. PropTypes is defined as camelCased propTypes, but PropTypes is used incorrectly above.

Should be fixed to propTypes.


CONSTANT_CONDITION

Condition 'site' is always false at this point because it conflicts with the previous condition at line 10.

client/lib/paths/index.js

/**
 * Internal dependencies
 */
import { login } from './login';

function editorPathFromSite( site ) {
    let path = '',
        siteSlug;

    if ( site ) {
        siteSlug = ( typeof site === 'object' ) ? site.slug : site;
        path = '/' + siteSlug;
    } else if ( site && typeof site === 'object' ) {
        path = '/' + site.ID + '/new';

site at line 13 is a falsy value because it is in the else branch of the condition at line 10.

I think that this block at line 13 is useless and should be removed.


CALL_NON_FUNC

Expression '`Key name passed into `' evaluates to a string value. But it is called as a function at this point.

client/state/utils.js

export const keyedReducer = ( keyName, reducer ) => {
    // some keys are invalid
    if ( 'string' !== typeof keyName ) {
        throw new TypeError( `Key name passed into ``keyedReducer`` must be a string but I detected a ${ typeof keyName }` );
    }

Template literal should be escaped like \` instead of ``. Above code throws a TypeError: "Key name passed into " is not a function.