5
\$\begingroup\$

I created a syntax highlighter in JavaScript. The language it highlights is AdBlock Filter Syntax, the language used to write filters for ad blocker extensions in browsers.

Shown here is the AdBlockSyntaxLine Class, which is the core class that dices a line of text into categories. These categories are used to do highlighting later.

The entire project can be found on my GitHub. And here is a link to the live version.

Screenshot

enter image description here

AdBlockSyntaxLine

"use strict";

import { Helper } from './Helper.js';

export class AdBlockSyntaxLine {
    string = "";
    toParse = "";
    syntax = {
        'uboPreParsingDirective': '', // !#
        'agHint': '', // !+
        'comment': '', // !
        'exception': '', // @@
        'exceptionRegEx': '', // @@/regex/
        'domainRegEx': '', // /regex/
        'domain': '',
        'option': '', // $
        'selectorException': '', // #@#
        'selector': '', // ##
        'htmlFilter': '', // ##^
        'htmlFilterException': '', // #@#^
        'abpExtendedSelector': '', // #?#
        'uboScriptlet': '', // ##+js()
        'uboScriptletException': '', // #@#+js()
        'abpSnippet': '', // #$#
        'actionOperator': '', // :style() :remove()
    };
    isValid = "not sure";
    errorHint = "";
    
    constructor(s) {
        this.string = s;
        this.toParse = this.string;
        
        try {
            this._categorizeSyntax();
        } catch(e) {
            // only catch what we want, let actual errors throw to console
            if ( e === true || e === false || e === "not sure" ) {
                this.isValid = e;
            } else {
                throw e;
            }
        }
        
        if ( this.isValid !== true ) {
            try {
                this._lookForErrors();
            } catch(e) {
                // only catch what we want, let actual errors throw to console
                if ( e === true || e === false || e === "not sure" ) {
                    this.isValid = e;
                } else {
                    throw e;
                }
            }
        }
        this._lookForMismatch();
    }
    
    _lookForErrors() {
        // no spaces in domains or domain regex
        if ( this.syntax['domainRegEx'] && this.syntax['domainRegEx'].search(/ /g) !== -1 ) {
            this.errorHint = "no spaces allowed in domains, exceptions, domainRegEx, or exceptionRegEx";
            throw false;
        }
        if ( this.syntax['domain'] && this.syntax['domain'].search(/ /g) !== -1 ) {
            this.errorHint = "no spaces allowed in domains, exceptions, domainRegEx, or exceptionRegEx";
            throw false;
        }
        if ( this.syntax['exceptionRegEx'] && this.syntax['exceptionRegEx'].search(/ /g) !== -1 ) {
            this.errorHint = "no spaces allowed in domains, exceptions, domainRegEx, or exceptionRegEx";
            throw false;
        }
        if ( this.syntax['exception'] && this.syntax['exception'].search(/ /g) !== -1 ) {
            this.errorHint = "no spaces allowed in domains, exceptions, domainRegEx, or exceptionRegEx";
            throw false;
        }
        
        // Delete regex. Regex is allowed to contain our special chars. When we do our searches, we don't want to get false positives.
        let s = this.string;
        s = s.replace(/^\/.*?[^\\]\//g, '');
        s = s.replace(/^@@\/.*?[^\\]\//g, '@@');
        
        // look for double selectors $ #@# ## ##^ #@#^ #?# ##+js( #@#+js( #$#
        // had to take out $, too many false positives, it's used in CSS and +js()
        let count = Helper.countRegExMatches(s, /\#@#|##|##\^|#@#\^|#\?#|##\+js\(|#@#\+js\(|#\$#/);
        if ( count > 1 ) {
            this.errorHint = "selector-ish syntax $ #@# ## ##^ #@#^ #?# ##+js( #@#+js( #$# is only allowed once per filter";
            throw false;
        }
        
        // look for double actionOperators
        count = Helper.countRegExMatches(s, /:style\(|:remove\(/);
        if ( count > 1 ) {
            this.errorHint = "actionOperators :style() :remove() are only allowed once per filter";
            throw false;
        }
        
        // actionOperators must be paired with a domain
        let domainPresent = (
            this.syntax['domain'] ||
            this.syntax['exception'] ||
            this.syntax['domainRegEx'] ||
            this.syntax['exceptionRegEx']
        );
        if ( this.syntax['actionOperator'] && ! domainPresent ) {
            this.errorHint = "actionOperators :style() :remove() must be used with a URL";
            throw false;
        }
        
        // actionOperators not allowed to be paired with ##+js( #@#+js( #$# $
        // TODO: probably also need to ban pairing with #@#|##|##^|#@#^|#?#| but so far :style() passes ubo validator, :remove() fails
        let bannedSyntaxPresent = (
            this.syntax['uboScriptlet'] ||
            this.syntax['uboScriptletException'] ||
            this.syntax['abpSnippet'] ||
            this.syntax['option']
        );
        let countActionOperators = Helper.countRegExMatches(s, /:style\(|:remove\(/);
        if ( bannedSyntaxPresent && countActionOperators ) {
            this.errorHint = "actionOperators :style() :remove() cannot be used with ##+js( #@#+js( #$# $";
            throw false;
        }
        
        // @@exceptions may not contain any selectors except options
        count = Helper.countRegExMatches(s, /\#@#|##|##\^|#@#\^|#\?#|##\+js\(|#@#\+js\(|#\$#|:style\(|:remove\(/);
        let exception = ( this.syntax['exception'] || this.syntax['exceptionRegEx'] );
        if ( exception && count ) {
            this.errorHint = "@@ statements may not contain selector-ish syntax $ #@# ## ##^ #@#^ #?# ##+js( #@#+js( #$# or action operators :style() :remove()"
            throw false;
        }
        
        // ##+js() #@#+js() :style() :remove() must end in )
        let lastChar = s.right(1);
        let shouldEndInParenthesis = ( this.syntax['uboScriptlet'] ||  this.syntax['uboScriptletException'] ||  this.syntax['actionOperator'] );
        if ( shouldEndInParenthesis && lastChar !== ')' ) {
            this.errorHint = "##+js() #@#+js() :style() :remove() must end in )"
            throw false;
        }
    }
    
    /** Takes the values in the this.syntax array and builds them into a string. Then makes sure that string matches the input string. If these don't match, this is a pretty sure sign there's a bug. */
    _lookForMismatch() {
        let lineString = "";
        for ( let key in this.syntax ) {
            lineString += this.syntax[key];
        }
        
        if ( lineString !== this.string ) {
            this.isValid = "mismatch";
        }
    }
    
    /** dice syntax string up into categories: comment !, exception @@, domain, option $, selectorException #@#, selector ##, abpExtendedSelector #?#, actionoperator :style(), abpSnippet #$#, etc. */
    _categorizeSyntax() {
        this._lookForComments();
        this._lookForDomains();
        // lookForActionOperators needs to come before lookForSelectors, even though actionOperators appear after selectors in the string.
        this._lookForActionOperators();
        this._lookForSelectors();
    }
        
    _lookForComments() {    
        // uboPreParsingDirective !#
        if ( this.toParse.left(2) === "!#" ) {
            this.syntax['uboPreParsingDirective'] = this.string;
            throw "not sure";
        }
        
        // agHint !+
        if ( this.toParse.left(2) === "!+" ) {
            this.syntax['agHint'] = this.string;
            throw "not sure";
        }
        
        // comment ! [
        if ( this.string.left(1) === '!' || this.string.left(1) === '[' ) {
            this.syntax['comment'] = this.string;
            throw true;
        }
    }
    
    _lookForDomains() {
        // domainRegEx /regex/
        let matchPos = this.toParse.search(/^\/.*?[^\\]\//);
        let regExLookingStringFound = (matchPos !== -1);
        let toParse = this.toParse.replace(/^\/.*?[^\\]\//, '');
        let regEx = this.toParse.left(this.toParse.length - toParse.length);
        let selectorAfterRegEx = (toParse.search(/^(\$|#@#|##|##\^|#@#\^|#\?#|##\+js\(|#@#\+js\(|#\$#)/) !== -1);
        let nothingAfterRegEx = (toParse.length === 0);
        if ( regExLookingStringFound && (selectorAfterRegEx || nothingAfterRegEx) ) {
            this.syntax['domainRegEx'] = regEx;
            this.toParse = toParse;
            return;
        }
        
        // exceptionRegEx @@/regex/
        matchPos = this.toParse.search(/^@@\/.*?[^\\]\//);
        regExLookingStringFound = (matchPos !== -1);
        toParse = this.toParse.replace(/^@@\/.*?[^\\]\//, '');
        regEx = this.toParse.left(this.toParse.length - toParse.length);
        selectorAfterRegEx = (toParse.search(/^(\$|#@#|##|##\^|#@#\^|#\?#|##\+js\(|#@#\+js\(|#\$#)/) !== -1);
        nothingAfterRegEx = (toParse.length === 0);
        if ( regExLookingStringFound && (selectorAfterRegEx || nothingAfterRegEx) ) {
            this.syntax['domainRegEx'] = regEx;
            this.toParse = toParse;
            return;
        }
        
        // exception @@
        let domainException = false;
        if ( this.string.left(2) === '@@' ) {
            domainException = true;
        }
        
        // domain
        // parse until $ #@# ## #?# #$#
        // str.search returns first position, when searching from left to right (good)
        matchPos = this.toParse.search(/#@#|##|#\?#|#\$#|\$/);
        // if no categories after the domain
        if ( matchPos === -1 ) {
            this.syntax['domain'] = this.toParse;
            this.toParse = '';
        } else {
            this.syntax['domain'] = this.toParse.left(matchPos);
            this.toParse = this.toParse.slice(matchPos);
        }
        
        // exception @@ must have a domain
        if ( domainException && ! this.syntax['domain'] ) {
            this.errorHint = "exception @@ must have a domain";
            throw false;
        }
        
        // exception @@
        if ( domainException ) {
            this.syntax['exception'] = this.syntax['domain'];
            this.syntax['domain'] = "";
        }
    }
    
    _lookForSelectors() {
        // option $ (example: image)
        if ( this.toParse.left(1) === '$' ) {
            this.syntax['option'] = this.toParse;
            // OK to have nothing before it
            // Nothing allowed after it
            throw "not sure";
        }
        
        // abpSnippet #$# (example: log hello world!)
        if ( this.toParse.left(3) === "#$#" ) {
            this.syntax['abpSnippet'] = this.toParse;
            // Nothing allowed after it
            throw "not sure";
        }
        
        // uboScriptletException #@#+js(
        if ( this.toParse.left(7) === "#@#+js(" ) {
            this.syntax['uboScriptletException'] = this.toParse;
            // Nothing allowed after it
            throw "not sure";
        }
        
        // uboScriptlet ##+js(
        if ( this.toParse.left(6) === "##+js(" ) {
            this.syntax['uboScriptlet'] = this.toParse;
            
            // per ublock documentation, example.com##+js() when js() is empty is an error
            if ( this.syntax['uboScriptlet'] === "##+js()" ) {
                this.errorHint = "per ublock documentation, example.com##+js() when js() is empty is an error";
                throw false;
            }
            
            // Nothing allowed after it
            throw "not sure";
        }
        
        // htmlFilter ##^
        if ( this.toParse.left(3) === "##^" ) {
            this.syntax['htmlFilter'] = this.toParse;
            return;
        }
        
        // htmlFilterException #@#^
        if ( this.toParse.left(4) === "#@#^" ) {
            this.syntax['htmlFilterException'] = this.toParse;
            return;
        }
        
        // selectorException #@#
        if ( this.toParse.left(3) === "#@#" ) {
            this.syntax['selectorException'] = this.toParse;
            return;
        }
        
        // selector ##
        if ( this.toParse.left(2) === "##" ) {
            this.syntax['selector'] = this.toParse;
            return;
        }
        
        // abpExtendedSelector #?#
        if ( this.toParse.left(3) === "#?#" ) {
            this.syntax['abpExtendedSelector'] = this.toParse;
            return;
        }
    }
    
    _lookForActionOperators() {
        let matchPos = this.toParse.search(/(:style\(|:remove\().*\)$/);
        if ( matchPos !== -1 ) {
            this.syntax['actionOperator'] = this.toParse.slice(matchPos);
            this.toParse = this.toParse.left(matchPos);
        }
    }
    
    /** Gets a string with a JSON representation of the syntax categories. Also prints isValid and errorHint. */
    getJSON() {
        let s = "";
        s += "Filter = " + this.string + "\n";
        s += "Valid? = " + this.isValid + "\n";
        if ( this.errorHint ) {
            s += "Error Hint = " + this.errorHint + "\n";
        }
        s += JSON.stringify(this.syntax);
        // add enters after commas
        s = s.replace(/",/g, '",\n');
        return s;
    }
    
    /** Gets a string of the filter syntax, with HTML <span>s wrapped around each category of syntax. These <span>s will be used to highlight the text the correct color in the richTextBox. */
    getRichText() {
        let richText = "";
        let classes = "";
        for ( let key in this.syntax ) {
            classes = key;
            if ( ! this.isValid || this.isValid === "mismatch" ) {
                classes += " error";
            }
            if ( this.syntax[key] ) {
                let s = this.syntax[key];
                s = Helper.escapeHTML(s);
                s = s.replace(/ /g, "&nbsp;");
                richText += '<span class="' + classes + '">' + s + '</span>';
            }
        }
        return richText;
    }
}

Helper

"use strict";

export class Helper {
    static countRegExMatches(str, regExPattern) {
        regExPattern = new RegExp(regExPattern, "g");
        return ((str || '').match(regExPattern) || []).length;
    }

    static escapeHTML(unsafe) {
        return unsafe
            .replace(/&/g, "&amp;")
            .replace(/</g, "&lt;")
            .replace(/>/g, "&gt;")
            .replace(/"/g, "&quot;")
            .replace(/'/g, "&#039;");
    }
}

String.prototype

// TODO: figure out how to move this into its own file and import/export it. Currently, adding "export" to the beginning of it generates an error.
Object.assign(String.prototype, {
    /** @description "Testing 123".left(4) = "Test" */
    left(length) {
        return this.slice(0, length);
    },
    
    /** @description "Testing 123".right(3) = "123" */
    right(length) {
        return this.substr(this.length - length);
    },
});

Possible code smells

  • Code needs some optimization. Currently gets pretty slow if parsing more than 500 lines. I imagine use of RegEx is slowing things down a bit.
  • I don't use const. So far I am not liking JavaScript's insistence that variables be declared, and having to pick between let/const. Seems to require a lot of thought and require a lot of debugging, without providing much benefit.
  • If anybody knows how to put string.prototype in its own file and get it to work with the export keyword, that'd be awesome. I couldn't figure it out, so I put it in my main file as a workaround.
  • Try/Catch true/false/"not sure" feels like a code smell. I couldn't think of a better way to return across multiple functions though.
\$\endgroup\$
1

1 Answer 1

1
\$\begingroup\$

I don't think I can review all of that, but here are some thoughts to your questions and some general remarks.

const/let

I would guess this is simply that your programming style colliding with the trend in JavaScript towards functional programming where mutability is an anti-pattern and generally avoided.

The first step to using const is to avoid reusing variables. This on the first look seems like it would introduce a lot of superfluous variables, but usually alternative syntaxes or patterns can get around that.

Just one example: Instead of

let s = this.string;
s = s.replace(/^\/.*?[^\\]\//g, '');
s = s.replace(/^@@\/.*?[^\\]\//g, '@@');

one could write

const s = this.string;
const s1 = s.replace(/^\/.*?[^\\]\//g, '');
const s2 = s1.replace(/^@@\/.*?[^\\]\//g, '@@');

or (in this case) better would be

const s = this.string
            .replace(/^\/.*?[^\\]\//g, '')
            .replace(/^@@\/.*?[^\\]\//g, '@@');

String.prototype

Modifying a prototype is generally a bad idea. It affects all scripts globally and that's also the reason there is no way to "export" it: You can't limit it to just your script. It effects all scripts in the same environment, which can lead to unexpected errors.

In your case it's not really needed IMO anyway. Using left(length) doesn't give much more information than just slice(0, length), and right(length) can similarly be expressed as slice(-length).

If you do prefer separate functions just use regular functions.

BTW, x.left(2) === "##" can be better expressed as x.startsWith("##").

throw

Yeah, this is bad. Combined with setting the state of the class the execution and data flow is completely in-transparent for the reader. A better understandable (albeit verbose) way would be to have each function return a status and after calling it check if the status requires aborting. Something like this pseudo code:

_categorizeSyntax() {
    const commentStatus = this._lookForComments();
    if (isFinished(commentStatus)) { return commentStatus; }

    const domainStatus = this._lookForDomains();
    if (isFinished(domainStatus)) { return domainStatus; }

   // etc.
}

_lookForComments() {
    if ( this.toParse.left(2) === "!#" ) {
        this.syntax['uboPreParsingDirective'] = this.string;
        return { isValid: "not sure" };
    }    
    // etc.
 }

 isFinished(result) {
      return result.hasOwnProperty("isValid");
 }

class

A thing that plays into this is the use of a class. I believe having all functionality in the constructor and using the class as a "data dump" is an anti-pattern, but I can't find a proper name for it.

It would be better to put the logic in a regular function that in the end returns an plain data object containing the result of the parsing.

Generally functions are much better readable if they only reads its parameters (and not read from the "global" state) and only return data (instead of mutating the "global" state) - so-called "pure" functions.

The state could be, for example, instead passed around as a parameter (again pseudo code):

_categorizeSyntax({string: "The string to be parsed"}) // Inital state.

_categorizeSyntax(state) {
    const commentState = this._lookForComments(state);
    if (isFinished(commentState)) { return commentState; }

    const domainState = this._lookForDomains(commentState);
    if (isFinished(domainState)) { return domainState; }

   // etc.
   return state;
}

_lookForComments(state) {
    if ( state.string.left(2) === "!#" ) {
        return {
           ...state,
           syntax: { "uboPreParsingDirective": state.string },
           isValid: "not sure"
        };
    }    
    // etc.
    return state;
 }

 isFinished(result) {
      return result.hasOwnProperty("isValid");
 }

Another way to do it would be use the functional "either" pattern/monad, but that would too much here.

getRichText/escapeHTML

Some final thoughts about getRichText and escapeHTML (although there aren't seemed to used in this code):

It would be better to use existing libraries or built in functionality for standardized things like escapeHTML. If this code runs in a browser it would make sense to let the browser build the HTML. Depending what you are doing with the created HTML elements it also would make sense to directly return a list of DOM elements instead of HTML in strings. For example:

getRichText() {
        const elements = [];
        for ( let key in this.syntax ) {
            if ( this.syntax[key] ) {
                const span = document.createElement("span");

                span.classList.add(key);
                if ( ! this.isValid || this.isValid === "mismatch" ) {
                    span.classList.add("error");
                }

                // Use Unicode non-breaking space instead of HTML entity
                const text = this.syntax[key].replace(/ /g, "\u00A0");
                span.textContent = text;

                elements.push(span);
            }
        }
        return elements;
    }
}

If needed the text representation of a DOM element can be got with .outerHTML.

And if the code isn't running in a browser or you really want a string instead of DOM elements, then you should consider a template engine.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you very much for your awesome feedback. The quality and positivity of the answers I get on Code Review is superb. I will work my way through your list and incorporate most of these. I really appreciate it. \$\endgroup\$ Commented Jul 7, 2020 at 20:41
  • \$\begingroup\$ Also, I started a bounty on StackOverflow for a question related to this program. Feel free to check it out. stackoverflow.com/questions/62705449/… \$\endgroup\$ Commented Jul 8, 2020 at 0:49

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.