Avoid resolveTargets call if browsers is an empty array

[dev-itsheng]

In the process of using babel, I found that when browserslistConfigFile is not set to false and browsersList is not set, babel will still call the related methods of browsersList and caniuse-lite, I think this is unnecessary, Because browsers is an empty array at this time, it is a truthy value, so I modified the judgment condition to not handle it when it is undefined or an empty array.

Q	A
Fixed Issues?	No
Patch: Bug Fix?	Yes
Major: Breaking Change?	No
Minor: New Feature?	No
Tests Added + Pass?	No, because it's hard to see if the test works as expected without side effects, I'll think about how to design the test if necessary.
Documentation PR Link
Any Dependency Changes?	No
License	MIT

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51477/

[JLHwung]

You can add a test after

babel/packages/babel-helper-compilation-targets/test/targets-parser.spec.js

Lines 287 to 301 in aa5ff36

	(process.env.BABEL_8_BREAKING ? it.skip : it)(
	"'intersect' behaves like 'true' if no browsers are specified - Babel 7",
	() => {
	expect(getTargets({ esmodules: "intersect" })).toEqual(
	getTargets({ esmodules: true }, { ignoreBrowserslistConfig: true }),
	);
	},
	);
	(process.env.BABEL_8_BREAKING ? it : it.skip)(
	"'intersect' behaves like no-op if no browsers are specified",
	() => {
	expect(getTargets({ esmodules: "intersect" })).toEqual(getTargets({}));
	},
	);

The behaviour of getTargets({ esmodules: "intersect", browsers: [] })
is not covered by current test suites. I think it should behave as same as getTargets({ esmodules: "intersect" }).

[dev-itsheng]

I'm sorry I didn't reply to you until a week later. On the one hand, I was too busy with work, on the other hand, I was seriously thinking about how to test this code. Here is my opinion:

1. According to your point of view, the added test code should be like this:

(process.env.BABEL_8_BREAKING ? it.skip : it)(
      "'browsers' option will have no effect if it is an empty array - Babel 7",
      () => {
        expect(getTargets({ esmodules: "intersect", browsers: [] })).toEqual(
          getTargets({ esmodules: "intersect" }),
        );
      },
    );

I added it and this code passed the test.

But I don't think the two are necessarily equivalent. Observe the code for getTargets:

  let { browsers, esmodules } = inputTargets;
  const { configPath = "." } = options;

  validateBrowsers(browsers);

  const input = generateTargets(inputTargets);
  let targets: TargetsTuple = validateTargetNames(input);

  const shouldParseBrowsers = !!browsers;
  const hasTargets = shouldParseBrowsers || Object.keys(targets).length > 0;
  const shouldSearchForConfig =
    !options.ignoreBrowserslistConfig && !hasTargets;
  // ...

For browsers, whether its value is undefined or [], the value of hasTargets is false, and the value of shouldSearchForConfig depends on options.ignoreBrowserslistConfig (in the test code, it is undefined), in this case true.

  if (!browsers && shouldSearchForConfig) {
    browsers = browserslist.loadConfig({
      config: options.configFile,
      path: configPath,
      env: options.browserslistEnv,
    });
    if (browsers == null) {
      if (process.env.BABEL_8_BREAKING) {
        // In Babel 8, if no targets are passed, we use browserslist's defaults
        // and exclude IE 11.
        browsers = ["defaults, not ie 11"];
      } else {
        // If no targets are passed, we need to overwrite browserslist's defaults
        // so that we enable all transforms (acting like the now deprecated
        // preset-latest).
        browsers = [];
      }
    }
  }

In the scenario of getTargets({ esmodules: "intersect" }), the value of browsers is undefined, enter if, and browserslist.loadConfig in the test environment is reassigned to undefined, == null is true, enter if, reassign it to [], and in getTargets({ esmodules: "intersect", browsers: [] }), will not enter if, Its value is [].

That said, the behavior is consistent here without getting to where I've changed it, but in practice it also depends on the value of configFile, if browsers is undefined and browserslist. loadConfig returns a non-empty array, which behaves inconsistently with [].

To prove that undefined is equivalent to [] when passed in, we have to consider various boundary conditions and user configuration, so I have a question: Do I need to write separate test cases for each condition? Or make their scenarios sufficiently clear in the test case description?

The behavior that I think is more necessary to test is that the behavior of undefined and [] can be consistent when executing here, so I added another test case:

(process.env.BABEL_8_BREAKING ? it.skip : it)(
      "The final 'browsers' handled variable will have no effect if it is an empty array - Babel 7",
      () => {
        expect(getTargets({ esmodules: "intersect", browsers: [] })).toEqual(
          getTargets(
            { esmodules: "intersect" },
            { ignoreBrowserslistConfig: true },
          ),
        );
      },
    );

It also passed the test.

2. How to verify my changes

As mentioned above, the reason for this PR is that I don't want to execute resolveTargets when browsers is [], because it will call the related methods of browserslist and caniuse-lite, which takes some time time, this is not necessary, what I need to prove is that the following code, when browsers is passed in as [], should have no effect, that is, whether it is entered in if or Without entering, it should behave the same:

if (browsers) {
    const queryBrowsers = resolveTargets(browsers, options.browserslistEnv);

    if (esmodules === "intersect") {
      for (const browser of Object.keys(queryBrowsers)) {
        const version = queryBrowsers[browser];

        if (ESM_SUPPORT[browser]) {
          queryBrowsers[browser] = getHighestUnreleased(
            version,
            semverify(ESM_SUPPORT[browser]),
            browser,
          );
        } else {
          delete queryBrowsers[browser];
        }
      }
    }

    targets = Object.assign(queryBrowsers, targets);
  }

Let's first observe the code of resolveTargets:

function resolveTargets(queries: Browsers, env?: string): Targets {
  const resolved = browserslist(queries, {
    mobileToDesktop: true,
    env,
  });
  return getLowestVersions(resolved);
}

It calls the browserslist function, which also returns an Empty array [].

getLowerestVersions returns {}:

function getLowestVersions(browsers: Array<string>): Targets {
  return browsers.reduce((all: any, browser: string): any => {
     // ...
  }, {});
}

When queryBrowsers is {}, the following if will not affect the result whether it is executed or not, and Object.assign(queryBrowsers, targets) is also equivalent to target. To sum up, this modification There will be no impact.

[JLHwung]

Thanks for the detailed explainer. It is very helpful.

That said, the behavior is consistent here without getting to where I've changed it

I agree. The unit test is to ensure consistent behaviour. Whether resolveTargets is called is implementation details somehow hidden from end users. Technically we can mock the browserslist and asserts that resolveTargets is not called, when it doesn't have to be, but I am cool with that.

The new test case looks good to me.

[dev-itsheng]

Yes, I think it would be a good idea to modify the definition of the browserslist package so that we can observe whether the process is executed or not.

I looked up the usage of Jest and found that it is possible to use jest.mock to mock the return value of a module, but I am in the code Executed in , it reported an error saying 'jest.mock is not a function', then I printed the variable jest and found that it only has two methods, fn and spyOn. I realized that this jest may not be a global variable injected automatically by Jest, but a custom one, and sure enough I found the modification in the global-setup.js file.

But when trying to add the mock attribute to globalThis.jest, there is no corresponding method in jest-mock, I want to use @jest/globals, but no matter in global-setup.js or targets-parser.spec.js They all report the error Do not import '@jest/globals' outside of the Jest test environment., so I don't know what to do for the time being. Do you know any good solutions?

In addition, I have commented the changes in the source code to make it easier to understand.

[nicolo-ribaudo]

Thanks! We use a custom Jest setup and run our tests on native ESM, which doesn't easily support mocking.

[nicolo-ribaudo]

I think this test has the same behavior in Babel 7 and Babel 8:

Suggested change

	(process.env.BABEL_8_BREAKING ? it.skip : it)(
	"'browsers' option will have no effect if it is an empty array - Babel 7",
	it("'browsers' option will have no effect if it is an empty array",

also the other one.

[dev-itsheng]

Yes, In your opinion, the difference of Babel 7 and Babel 8 is only browsers variable:

if (browsers == null) {
      if (process.env.BABEL_8_BREAKING) {
        // In Babel 8, if no targets are passed, we use browserslist's defaults
        // and exclude IE 11.
        browsers = ["defaults, not ie 11"];
      } else {
        // If no targets are passed, we need to overwrite browserslist's defaults
        // so that we enable all transforms (acting like the now deprecated
        // preset-latest).
        browsers = [];
      }
    }

So if the result isn't have any effect of this, this test can also run on Babel 8.

I will update this commit later.

Thanks for comment.

[dev-itsheng]

Observe the following code:

(process.env.BABEL_8_BREAKING ? it.skip : it)(
 "'browsers' option will have no effect if it is an empty array - Babel 7", () => {
   expect(getTargets({ esmodules: "intersect", browsers: [] })).toEqual(
     getTargets({ esmodules: "intersect" }),
   );
 },
);

it("The final 'browsers' handled variable will have no effect if it is an empty array", () => {
  expect(getTargets({ esmodules: "intersect", browsers: [] })).toEqual(
    getTargets(
      { esmodules: "intersect" },
      { ignoreBrowserslistConfig: true },
    ),
  );
});

When ignoreBrowserslistConfig is set to true, no action is taken, but when this variable is not set and browsers is not set, babel7 and babel8 behave differently.

So in my opinion, the first test can not remove the conditional judgment, the second test can.

[dev-itsheng]

During this time, I have searched for a lot of information and made various attempts, but there is still no way to modify the imported browserslist module without modifying the existing test framework code of babel.

jest.spyOn can only mock a certain property of the imported module, while the browserslist module directly imports a function, which is read-only according to the rules of ESModule, and we cannot modify it.

So, we can only prove that it was called by making some side effects, and nothing can be done about the test cases that do not perform side effects. For the former, I tried adding a test. For the latter, I'm still reluctant to give up, but try to list the pseudocode below, it might work if I find a suitable way later:

it("'resolveTargets' will not be called if 'browsers' is an empty array", () => {
  getTargets({ esmodules: "intersect", browsers: [] });
  expect(/* the number of browserslist function called */).toBe(0);
});

(process.env.BABEL_8_BREAKING ? it : it.skip)(
  "'resolveTargets' will not be called if 'browsers' is not defined - Babel 7", () => {
    getTargets({ esmodules: "intersect" });
    expect(/* the number of browserslist function called */).toBe(0);
  }
);

[nicolo-ribaudo]

Jest has native support for this:

expect(() => getTargets({ esmodules: "intersect", browsers: ["test"] })).toThrow();

[dev-itsheng]

Great idea, I've fixed it.

[nicolo-ribaudo]

The tests are good enough; we only have to check that the old behavior is preserved!