Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Avoid resolveTargets call if browsers is an empty array #14294

Merged
merged 15 commits into from Mar 14, 2022
Merged

Avoid resolveTargets call if browsers is an empty array #14294

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
bdc7fdf
fix: remove `resolveTargets` call if browsers is empty
dev-itsheng Feb 22, 2022
bba433a
Merge branch 'babel:main' into patch-1
dev-itsheng Mar 1, 2022
c03d061
fix: added validation for `browsers` values `[]` and `undefined`
dev-itsheng Mar 2, 2022
32c3aa7
Merge branch 'patch-1' of https://github.com/dev-itsheng/babel
dev-itsheng Mar 2, 2022
0183f3f
Merge branch 'babel:main' into patch-1
dev-itsheng Mar 4, 2022
8e9453f
fix: added some comment of changed code.
dev-itsheng Mar 4, 2022
426d2c7
Merge branch 'patch-1' of https://github.com/dev-itsheng/babel
dev-itsheng Mar 4, 2022
3eea970
fix: fixed some test preconditions, and added a test to see if a func…
dev-itsheng Mar 13, 2022
6944235
Merge branch 'babel:main' into patch-1
dev-itsheng Mar 13, 2022
6a4dab1
Merge branch 'patch-1' of https://github.com/dev-itsheng/babel
dev-itsheng Mar 13, 2022
57669c6
fix: Fix test error
dev-itsheng Mar 13, 2022
a3fe073
fix: Fix code style
dev-itsheng Mar 13, 2022
2d531e5
fix: Fix ESLint error of code style
dev-itsheng Mar 13, 2022
6527dc0
fix: Refactoring test code using Jest's `toThrow` syntax.
dev-itsheng Mar 13, 2022
5a5af58
fix: Re-fix ESLint error of code style
dev-itsheng Mar 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/babel-helper-compilation-targets/src/index.ts
View file
Open in desktop
Expand Up @@ -219,7 +219,7 @@ export default function getTargets(
esmodules = false;
}

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

if (esmodules === "intersect") {
Expand Down
21 changes: 21 additions & 0 deletions packages/babel-helper-compilation-targets/test/targets-parser.spec.js
View file
Open in desktop
Expand Up @@ -293,6 +293,27 @@ describe("getTargets", () => {
},
);

(process.env.BABEL_8_BREAKING ? it.skip : it)(
"'browsers' option will have no effect if it is an empty array - Babel 7",
Comment on lines +296 to +297
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

(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 },
),
);
},
);

(process.env.BABEL_8_BREAKING ? it : it.skip)(
"'intersect' behaves like no-op if no browsers are specified",
() => {
Expand Down