Fix validateOptions to report when secondary option object is an empty object or null

[ashleemboyer]

What steps are needed to reproduce the bug?

First, set up a rule that requires via validateOptions that the secondary option object is defined. I have an example here: https://github.com/ashleemboyer/stylelint-validate-options-test/blob/main/disallow-variable-name.js#L28-L44.

Second, either with Jest tests or the CLI tool & a config file, try running Stylelint with an empty object as the secondary option to the rule. I have an example here: https://github.com/ashleemboyer/stylelint-validate-options-test/blob/main/disallow-variable-name.test.js#L12-L28.

What Stylelint configuration is needed to reproduce the bug?

For a custom rule named disallow-variable-name, for example, two configurations that don't go as expected:

Secondary option is an empty object:

{
  "plugins": ["./disallow-variable-name.js"],
  "rules": {
    "my-test/disallow-variable-name": [true, {}],
  },
}

Secondary option is null:

{
  "plugins": ["./disallow-variable-name.js"],
  "rules": {
    "my-test/disallow-variable-name": [true, null],
  },
}

How did you run Stylelint?

With the standalone lint in a Jest test: https://github.com/ashleemboyer/stylelint-validate-options-test/blob/main/disallow-variable-name.test.js

Which version of Stylelint are you using?

14.9.1

What did you expect to happen?

I would expect validateOptions to treat an empty object the same way it treats undefined and an empty array as "nothing possible" here:

stylelint/lib/utils/validateOptions.js

Lines 73 to 74 in e870430

	const nothingPossible =
	possible === undefined || (Array.isArray(possible) && possible.length === 0);

.

What actually happened?

When the suggested steps for writing a plugin are followed for using validateOptions (code block below), and the rule returns early if validateOptions returns a falsy value, the rest of the rule continues to run. If a rule depends on and expects a certain option to be set, a runtime error happens where that option is referenced.

See example error related to other links I've shared: https://github.com/ashleemboyer/stylelint-validate-options-test/runs/6941567039?check_suite_focus=true#step:4:17

module.exports = stylelint.createPlugin(
  ruleName,
  (primaryOption, secondaryOptionObject) => {
    return (postcssRoot, postcssResult) => {
      const validOptions = stylelint.utils.validateOptions(
        postcssResult,
        ruleName,
        {
          /* .. */
        }
      );

      if (!validOptions) {
        return;
      }

      // ... some logic ...
      stylelint.utils.report({
        /* .. */
      });
    };
  }
);

Does the bug relate to non-standard syntax?

No response

Proposal to fix the bug

Update nothingPossible in validateOptions to check for empty objects, like it checks for empty arrays. Also proposes checking possible === null in addition to possible === undefined since a value of null can cause runtime errors too.

Current definition (link to code)):

const nothingPossible =
  possible === undefined ||
  (Array.isArray(possible) && possible.length === 0);

Proposed definition:

const nothingPossible =
  possible === undefined ||
  possible === null ||
  (Array.isArray(possible) && possible.length === 0) ||
  (isPlainObject(possible) && Object.keys(possible).length === 0);

[ybiquitous]

@ashleemboyer Thanks for writing the detailed report and using the template! Your proposal sounds good to me. 👍🏼

I've labeled the issue as ready to implement. Please consider contributing if you have time.

[ashleemboyer]

@ybiquitous I’d be happy to help out with this one! ☺️

[ashleemboyer]

Hey again @ybiquitous! I've started working on this, and began by adding some test cases for some of the behavior I expected above. Things aren't going how I expected, and I'm not sure if it's because I'm misunderstanding the existing code or not.

I have one question to start with. 😊

In validate, what is the purpose of immediately checking actual === null and arrayEqual(actual, [null]), and returning early if either of those conditions is true?

[ybiquitous]

Hi @ashleemboyer, thanks for touching on this issue. 😊

null means to turn off a rule. See the doc below:

stylelint/docs/user-guide/configure.md

Line 40 in bd8c7fd

- `null` (to turn the rule off)

[ybiquitous]

This is an old issue, but still valid. I'll address this.