Commit 67e93522 authored by Mark Banner's avatar Mark Banner
Browse files

Bug 1766238 - Disallow more than one argument to ChromeUtils.import via ESLint. r=mossop

Depends on D144562

Differential Revision: https://phabricator.services.mozilla.com/D144563
parent 1f8462cf
Loading
Loading
Loading
Loading
+3 −7
Original line number Diff line number Diff line
reject-chromeutils-import-params
================================

``ChromeUtils.import`` can be called with two arguments, however these are now
largely deprecated.

The use of object destructuring is preferred over the second parameter being
``this`` or ``{}``.

Using explicit exports is preferred over the second parameter being ``null``.
``ChromeUtils.import`` used to be able to be called with two arguments, however
the second argument is no longer supported. Exports from modules should now be
explicit, and the imported symbols being accessed from the returned object.

Examples of incorrect code for this rule:
-----------------------------------------
+13 −51
Original line number Diff line number Diff line
@@ -41,49 +41,12 @@ module.exports = {
          isIdentifier(callee.property, "import") &&
          node.arguments.length >= 2
        ) {
          let targetObj = node.arguments[1];
          if (targetObj.type == "Literal" && targetObj.raw == "null") {
            context.report(
              node,
              "ChromeUtils.import should not be called with (..., null) to " +
                "retrieve the JSM global object. Rely on explicit exports instead."
            );
          } else if (targetObj.type == "ThisExpression") {
          context.report({
            node,
              message:
                "ChromeUtils.import should not be called with (..., this) to " +
                "retrieve the JSM global object. Use destructuring instead.",
            message: "ChromeUtils.import only takes one argument.",
            suggest: [
              {
                  desc: "Use destructuring for imports.",
                  fix: fixer => {
                    let source = context.getSourceCode().getText(node);
                    let match = source.match(
                      /ChromeUtils.import\(\s*(".*\/(.*).jsm?")/m
                    );

                    return fixer.replaceText(
                      node,
                      `const { ${match[2]} } = ChromeUtils.import(${match[1]})`
                    );
                  },
                },
              ],
            });
          } else if (
            targetObj.type == "ObjectExpression" &&
            targetObj.properties.length == 0
          ) {
            context.report({
              node,
              message:
                "Passing an empty object to ChromeUtils.import is unnecessary",
              suggest: [
                {
                  desc:
                    "Passing an empty object to ChromeUtils.import is " +
                    "unnecessary - remove the empty object",
                desc: "Remove the unnecessary parameters.",
                fix: fixer => {
                  return fixer.removeRange(
                    getRangeAfterArgToEnd(context, 0, node.arguments)
@@ -93,7 +56,6 @@ module.exports = {
            ],
          });
        }
        }
      },
    };
  },
+1 −1
Original line number Diff line number Diff line
{
  "name": "eslint-plugin-mozilla",
  "version": "2.12.0",
  "version": "2.12.1",
  "lockfileVersion": 1,
  "requires": true,
  "dependencies": {
+1 −1
Original line number Diff line number Diff line
{
  "name": "eslint-plugin-mozilla",
  "version": "2.12.0",
  "version": "2.12.1",
  "description": "A collection of rules that help enforce JavaScript coding standard in the Mozilla project.",
  "keywords": [
    "eslint",
+27 −60
Original line number Diff line number Diff line
@@ -16,11 +16,19 @@ const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 8 } });
// Tests
// ------------------------------------------------------------------------------

function invalidError() {
  let message =
    "ChromeUtils.import should not be called with (..., null) to " +
    "retrieve the JSM global object. Rely on explicit exports instead.";
  return [{ message, type: "CallExpression" }];
function invalidError(suggested) {
  return [
    {
      message: "ChromeUtils.import only takes one argument.",
      type: "CallExpression",
      suggestions: [
        {
          desc: "Remove the unnecessary parameters.",
          output: suggested,
        },
      ],
    },
  ];
}

ruleTester.run("reject-chromeutils-import-params", rule, {
@@ -28,7 +36,9 @@ ruleTester.run("reject-chromeutils-import-params", rule, {
  invalid: [
    {
      code: 'ChromeUtils.import("resource://some/path/to/My.jsm", null)',
      errors: invalidError(),
      errors: invalidError(
        `ChromeUtils.import("resource://some/path/to/My.jsm")`
      ),
    },
    {
      code: `
@@ -36,65 +46,22 @@ ChromeUtils.import(
  "resource://some/path/to/My.jsm",
  null
);`,
      errors: invalidError(),
      errors: invalidError(`
ChromeUtils.import(
  "resource://some/path/to/My.jsm"
);`),
    },
    {
      code: 'ChromeUtils.import("resource://some/path/to/My.jsm", this)',
      errors: [
        {
          suggestions: [
            {
              desc: "Use destructuring for imports.",
              output: `const { My } = ChromeUtils.import("resource://some/path/to/My.jsm")`,
            },
          ],
        },
      ],
      errors: invalidError(
        `ChromeUtils.import("resource://some/path/to/My.jsm")`
      ),
    },
    {
      code: 'ChromeUtils.import("resource://some/path/to/My.js", this)',
      errors: [
        {
          suggestions: [
            {
              desc: "Use destructuring for imports.",
              output: `const { My } = ChromeUtils.import("resource://some/path/to/My.js")`,
            },
          ],
        },
      ],
    },
    {
      code: `
ChromeUtils.import(
  "resource://some/path/to/My.jsm",
  this
);`,
      errors: [
        {
          suggestions: [
            {
              desc: "Use destructuring for imports.",
              output: `
const { My } = ChromeUtils.import("resource://some/path/to/My.jsm");`,
            },
          ],
        },
      ],
    },
    {
      code: 'ChromeUtils.import("resource://some/path/to/My.js", {})',
      errors: [
        {
          suggestions: [
            {
              desc:
                "Passing an empty object to ChromeUtils.import is unnecessary - remove the empty object",
              output: `ChromeUtils.import("resource://some/path/to/My.js")`,
            },
          ],
        },
      ],
      code: 'ChromeUtils.import("resource://some/path/to/My.jsm", foo, bar)',
      errors: invalidError(
        `ChromeUtils.import("resource://some/path/to/My.jsm")`
      ),
    },
  ],
});