deprecate noHighlighting and replace it with reporterOptions.highlight #3701

[Mia-jeong]

Description

I added a deprecation notice for noHighlighting in browser and replace it with reporterOptions.highlight as mentioned in #3701

Description of the Change

1.

If a user supplies {noHighlighting: true} to mocha.setup() or calls mocha.noHighlighting() in the browser, we should print a deprecation message along the lines of "noHighlighting is deprecated; provide {reporterOption: {highlight: false}} to mocha.setup().

while I was working on it. I found out a bug related to noHighlighting.

Problem

Even though you pass false to noHighlighting in mocha.setup() like below,

mocha.setup({ui: 'bdd', noHighlighting: false});

noHighlighting turns into true, therefore we can't highlight text.

Reason

I tracked down the code to deal with it, and found out a reason about it.
in mocha.setup in browser-entry.js, you can see the code below. if options are passed, it calls the function named after the options.

 this[opt](opts[opt]);

for example, if we supply noHighlighting to mocha.setup(), noHighlighting() function will be invoked.
In noHighlighting() function it set true to noHighlighting, No matter which value were passed. Here's the code in /lib/mocha.js

Mocha.prototype.noHighlighting = function() {
  this.options.noHighlighting = true;
  return this;
};

Solution

I changed Mocha.prototype.noHighlighting() in lib/mocha.js like one below.
I let function get a parameter called enableHighlight.
If we call mocha.noHighlighting(), it will be undefined, then reporterOptions.highlight is set as false.
If we set value to noHighlighting in mocha.setup(), reporterOptions.highlight is set as the value depending on which value are supplied by user.

As you can see it, I added a deprecation notice inside the function as well.

Mocha.prototype.noHighlighting = function(enableHighlight) {
  utils.deprecate(
    'noHighlighting is deprecated; provide {reporterOption: {highlight: false}} to mocha.setup().'
  );
  this.options.reporterOptions = this.options.reporterOptions || {};
  this.options.reporterOptions.highlight =
    enableHighlight !== undefined && enableHighlight !== true;
  return this;
};

In addition, In case they pass {reporterOptions: {highlight: false}} to mocha.setup(), to avoid throwing an error I added conditional statements below in browser-entry.js as well. Because there's no function called reporterOptions

      if (opt === 'reporterOptions') {
        this.options.reporterOptions = opts[opt];
      } else {
        this[opt](opts[opt]);
      }

2.

The html reporter's constructor should accept a reporterOptions object, which may be undefined, and may have a boolean highlight property. If the property is anything other than explicitly false, default to true. in order to set reporterOptions.highlight value to options

I add the code to set the default value to it. so if they didn't pass false to reporterOptions.highlight, it will be true as default.

var enableHighlight = true;
if (
  options.reporterOptions &&
  options.reporterOptions.hasOwnProperty('highlight') &&
  !options.reporterOptions.highlight
) {
  enableHighlight = false;
}

3.

The reporter's constructor should execute Mocha.utils.highlightTags('code') when the end event is emitted by the runner property.

to run it, I added the code below in html.js. The value of enableHighlight is going to be set as mentioned in 2

  runner.once(EVENT_RUN_END, function() {
    if (enableHighlight) {
      utils.highlightTags('code');
    }
  });

I didn't check out whether document exists or not, because in html.js , I found out the code to throw an error when there's no document

  if (!root) {
    return error('#mocha div missing, add it to your document');
  }

4.

As I moved the highlighting code to html.js, I removed the duplicated checks below from browser-entry.js as mentioned in #3701

    var document = global.document;
    if (
      document &&
      document.getElementById('mocha') &&
      options.noHighlighting !== true
    ) {
      Mocha.utils.highlightTags('code');
    }

5.

add unit tests which assert the new behavior in test/unit/mocha.spec.js.

I modified test code of noHighlighting in test/unit/mocha.spec.js.
as a result of it, we can check out that if user calls mocha.noHighlighting(), reporterOptions.highlight will be false.

    it('should set the reporterOptions.highlight to false', function() {
      var mocha = new Mocha(opts);
      mocha.noHighlighting();
      expect(
        mocha.options.reporterOptions,
        'to have property',
        'highlight',
        false
      );
    });

In fact I wanted to add another test only for reporterOptions.highlight. but I wasn't able to do it. I added the code to set the default value to reporterOptions.highlight in html.js. so If I want to add a test, I think I should do it to html.spec.js, but There is nothing..

6.

add the description about it in documentation, so I added the text to index.md.

Here's what it looks like.

Benefits

As I stated above, I also fix the bug about noHighlighting(), so it's going to be helpful to deal with it as well.

Applicable issues

#3701

Thank you for reading my request :)

[outsideris]

No need to check opt value here. We can show a deprecation message whether noHighlighting is true or false.

[Mia-jeong]

oh I missed that, thank you so much.

[outsideris]

It should be enabled with noHighlighting before it removed.

[outsideris]

Not here. In browser-entry.js

[coveralls]

Coverage decreased (-0.1%) to 92.595% when pulling 7770b95 on Mia-jeong:fix-issue-3701-2 into 9584202 on mochajs:master.

[JoshuaKGoldberg]

👋 coming back to this @Mia-jeong, are you still interested in working on this PR? As of #5027 there are again maintainers who can review it now. No worries if you no longer have time - but if you do that'd be great!

[JoshuaKGoldberg]

Closing for now to keep our queue small, as there's an unresolved comment: https://github.com/mochajs/mocha/pull/4049/files#r332832395. But if you end up having time to work on this again @Mia-jeong we'd love to see you re-open this PR or post a new one.

And if anybody else sees this & sends their own, please include a co-authored-by attribution to credit @Mia-jeong for this work.