babel / babel

🐠 Babel is a compiler for writing next generation JavaScript.
https://babel.dev
MIT License
42.99k stars 5.59k forks source link

RegExp groups broken on `v7.13.x` versions #12876

Open yoav-lavi opened 3 years ago

yoav-lavi commented 3 years ago

Bug Report

Current behavior The latest v7.13.x version seem to break RegExp(...).exec(...).groups functionality. It is undefined for a correctly built RegExp when using these versions, but works correctly when setting:

{
  "devDependencies": {
      "@babel/cli": "7.12.17",
      "@babel/core": "7.12.17",
      "@babel/plugin-transform-runtime": "7.12.17",
      "@babel/preset-env": "7.12.17",
      "@babel/preset-typescript": "7.12.17",
  },
  "resolutions": {
      "@babel/cli": "7.12.17",
      "@babel/core": "7.12.17",
      "@babel/plugin-transform-runtime": "7.12.17",
      "@babel/preset-env": "7.12.17",
      "@babel/preset-typescript": "7.12.17",
   }
 }

Input Code

RegExp(/*...*/).exec(/*...*/).groups

Expected behavior

.groups should be defined for correct regexes

Babel Configuration (babel.config.js, .babelrc, package.json#babel, cli command, .eslintrc)

const test = process.env.BABEL_ENV === 'test';
const useESModules = test ? {} : { useESModules: false };

module.exports = {
  sourceType: 'module',
  presets: ['@babel/preset-typescript', '@babel/preset-react', '@babel/preset-env'],
  plugins: [['@babel/plugin-transform-runtime', { ...useESModules }], 'lodash'],
};

Environment

babel-bot commented 3 years ago

Hey @yoav-lavi! We really appreciate you taking the time to report an issue. The collaborators on this project attempt to help as many people as possible, but we're a limited number of volunteers, so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack community that typically always has someone willing to help. You can sign-up here for an invite."

nicolo-ribaudo commented 3 years ago

I cannot reproduce this with your config.

If I try to compile this code:

new RegExp("(?<foo>.)").exec("a").groups;

babel leaves it as-is, so it works.

Could you provide a repo I can clone to reproduce the bug :pray:

babel-bot commented 3 years ago

Hi @yoav-lavi! This issue is missing some important information we'll need to be able to reproduce this issue.

Please understand that we receive a high volume of issues, and there are only a limited number of volunteers that help maintain this project. The easier it is for us to decipher an issue with the info provided, the more likely it is that we'll be able to help.

Please make sure you have the following information documented in this ticket:

  1. Your Babel configuration (typically from .babelrc or babel.config.js)
  2. The current (incorrect) behavior you're seeing
  3. The behavior you expect
  4. A short, self-contained example

Please provide either a link to the problem via the repl, or if the repl is insufficient, a new and minimal repository with instructions on how to build/replicate the issue.

yoav-lavi commented 3 years ago

@nicolo-ribaudo this looks like something non deterministic since I can get it to pass or fail tests by adding console.log("#") before a try/catch block in which the regex runs.

When trying to create a demo project for this the issue happened on the older version as well.

Could you please try running

const userAgentRegex = /(?<type>(Chrome))\/(?<version>\d+)(.|\s)/;

const getBrowserInformation = (userAgent) => {
  try {
    const groups = RegExp(userAgentRegex).exec(userAgent)?.groups;
    return groups;
  } catch {
    return null;
  }
};

const browserInformation = getBrowserInformation(
  "Chrome/88.0.4324.182"
);

In a jest test using babel-jest? It seems to return undefined as browserInformation even though it should return {type: "Chrome", version: "88"} (and does in the browser / with some versions in some projects)

yoav-lavi commented 3 years ago

I'm also using the following:

  "browserslist": [
    "> 0.5%",
    "last 3 versions",
    "Firefox ESR",
    "not dead",
    "not ie > 0",
    "not edge < 80"
  ],

in package.json

yoav-lavi commented 3 years ago

The REPL seems to polyfill this when converting to an old version of node (9) but it doesn't end up working: https://babeljs.io/repl/#?browsers=%3E0.5%25%2C%0Alast%203%20versions%2C%0AFirefox%20ESR%2C%0Anot%20dead%2C%0Anot%20ie%20%3E%200%2C%0Anot%20edge%20%3C%2080&build=&builtIns=false&spec=false&loose=false&code_lz=MYewdgzgLgBArhApgJwIIHNFigJUZgDxgF4YB6ACgH4AeKATwAdEA-CgYQAtkQBbRAJQCAOpVoA3FBACW4FsIAmAagEUAdAB9hEAWQDcAKAMxQkWJigAhHgHckyAJJgAZiGS8AhlFlgSMCggoGFhQAiQsMADexjBQyPRRMTAm4NAw6DxwjBB-eOgAogSMAfbB2HmIBAJqlYjAJUGY2AJUahkgWRCGycnIiFBwyL7tnd0wAL4mXsCciT0wfQNDMGBwADZrY-MG43pAA&debug=false&forceAllTransforms=false&shippedProposals=false&circleciRepo=&evaluate=false&fileSize=true&timeTravel=false&sourceType=module&lineWrap=true&presets=env%2Creact%2Cstage-0%2Cstage-1%2Cstage-2%2Cstage-3%2Ctypescript&prettier=false&targets=Node-9&version=7.13.5&externalPlugins=

nicolo-ribaudo commented 3 years ago

Thanks I see the bug :+1:

nicolo-ribaudo commented 3 years ago

Actually, if you run your code (https://github.com/babel/babel/issues/12876#issuecomment-784306250) without Babel in Chrome's console it returns undefined :thinking:

yoav-lavi commented 3 years ago
Screen Shot 2021-02-23 at 17 25 06

Not for me

nicolo-ribaudo commented 3 years ago

Whops I forgot console.log :sweat_smile:

yoav-lavi commented 3 years ago

I actually didn't add it in the example but referred to looking at browserInformation, that's on me

the last screenshot didn't include the log -

Screen Shot 2021-02-23 at 17 26 24
yoav-lavi commented 3 years ago

@nicolo-ribaudo so just to make sure, do you have enough information / can see the bug now?

nicolo-ribaudo commented 3 years ago

Yes

nicolo-ribaudo commented 3 years ago

Oh this is not a regression, it's broken in every version (for example, 7.9.0).

The problem is that the only way we have to support this syntax in old browsers is to compile it away and subclassing the regexp.

This works, however when you call RegExp(...) you are telling the browser "please ignore any subclass, and give me an original regexp": this breaks Babel's polyfill. I'm not sure whether this is fixable :thinking:

As a workaround, you can avoid doing RegExp(...) and just use userAgentRegex.exec(userAgent).

yoav-lavi commented 3 years ago

It does work for me if I force 7.12.x in my project, which is a bit weird, maybe it was fixed and regressed after 7.9? It does behave really weirdly in some cases (like a log fixing the test) so I'm not sure if it's 7.12.x specifically or a set of specific circumstances aligning. It's worth mentioning that my targets are relatively recent so it may be a bit different than what happens in the REPL example. I can try using what you sent and see if that works though

yoav-lavi commented 3 years ago

Regarding the polyfill, may be worth having some documentation or warning about that since it looks like it does try to polyfill it and then fails silently

yoav-lavi commented 3 years ago

Not using RegExp() does it fact fix it for the latest version, still a bit weird that it works with 7.12.x

nicolo-ribaudo commented 3 years ago

It would be really helpful if you could share a repo where I can see that it works with an older version. I see the bug, but I don't understand how it could have possible been working in the past so I have no idea about what could have caused the regression :confused:

yoav-lavi commented 3 years ago

Unfortunately I can't share the original repo, and the test repo I made to reproduce this always reproduces the issue (making me suspect further that there's something other than the version that affects this, since when it did reproduce adding a log fixed it in some cases). I may be able to try to get it to work similarly later on

yoav-lavi commented 3 years ago

Got it, sending you a repo

yoav-lavi commented 3 years ago

https://github.com/yoav-lavi/babel-issue-reproduction There's a comment in the index file that tells you how to make it reproduce and stop reproducing, this specifically happens on 7.12

yoav-lavi commented 3 years ago

https://github.com/yoav-lavi/babel-issue-reproduction/blob/main/src/index.ts#L7

nicolo-ribaudo commented 3 years ago

Ok thanks!

yoav-lavi commented 3 years ago
Screen Shot 2021-02-23 at 18 13 34 Screen Shot 2021-02-23 at 18 13 02

I changed the instructions a bit, it looks like removing the comment itself also has something to do with it. Extremely weird, might be some babel-jest quirk. Adding the logs so you know I'm not making this up since a comment changing code output is extremely suspect.

yoav-lavi commented 3 years ago

It also seems to depend on whether BABEL_ENV = test was used (BABEL_ENV=test yarn run jest)

nicolo-ribaudo commented 3 years ago

Adding the logs so you know I'm not making this up

All these conditions seem so random that it's impossible to be so creative to make this up :stuck_out_tongue:

yoav-lavi commented 3 years ago
Screen Shot 2021-02-23 at 18 18 20

vs

Screen Shot 2021-02-23 at 18 18 12
nicolo-ribaudo commented 3 years ago

The bad news is that I still cannot reproduce it.

  1. What Node.js version are you using?
  2. Can you add console.log(getBrowserInformation.toString()); to see the compiled function in both cases?
yoav-lavi commented 3 years ago

with comment

    userAgent => {
      console.log("#");

      try {
        var _userAgentRegex$exec, _RegExp$exec, _userAgentRegex$exec2;

        // 123
        if (x) {
          console.log("#");
        }

        const groups = (_userAgentRegex$exec = userAgentRegex.exec(userAgent)) === null || _userAgentRegex$exec === void 0 ? void 0 : _userAgentRegex$exec.groups;
        console.log({
          regex: (_RegExp$exec = RegExp(userAgentRegex).exec(userAgent)) === null || _RegExp$exec === void 0 ? void 0 : _RegExp$exec.groups
        });
        console.log({
          regex: (_userAgentRegex$exec2 = userAgentRegex.exec(userAgent)) === null || _userAgentRegex$exec2 === void 0 ? void 0 : _userAgentRegex$exec2.groups
        });
        return groups;
      } catch {
        return null;
      }
    }
    userAgent => {
      console.log("#");

      try {
        var _userAgentRegex$exec, _RegExp$exec, _userAgentRegex$exec2;

        if (x) {
          console.log("#");
        }

        const groups = (_userAgentRegex$exec = userAgentRegex.exec(userAgent)) === null || _userAgentRegex$exec === void 0 ? void 0 : _userAgentRegex$exec.groups;
        console.log({
          regex: (_RegExp$exec = RegExp(userAgentRegex).exec(userAgent)) === null || _RegExp$exec === void 0 ? void 0 : _RegExp$exec.groups
        });
        console.log({
          regex: (_userAgentRegex$exec2 = userAgentRegex.exec(userAgent)) === null || _userAgentRegex$exec2 === void 0 ? void 0 : _userAgentRegex$exec2.groups
        });
        return groups;
      } catch {
        return null;
      }
    }

without comment

yoav-lavi commented 3 years ago
node --version
v15.9.0
yoav-lavi commented 3 years ago

Unfortunately it looks like adding the additional log also changes how it works

nicolo-ribaudo commented 3 years ago

Those two code snippets are the same when compiled (except for the comment), so it's even more unexpected that they behave differently: https://www.diffchecker.com/jKBQwBS1

yoav-lavi commented 3 years ago
   function getBrowserInformation(userAgent) {
      console.log("#");

      try {
        var _userAgentRegex$exec, _RegExp$exec, _userAgentRegex$exec2;

        if (x) {
          console.log("#");
        }

        var groups = (_userAgentRegex$exec = userAgentRegex.exec(userAgent)) === null || _userAgentRegex$exec === void 0 ? void 0 : _userAgentRegex$exec.groups;
        console.log({
          regex: (_RegExp$exec = RegExp(userAgentRegex).exec(userAgent)) === null || _RegExp$exec === void 0 ? void 0 : _RegExp$exec.groups
        });
        console.log({
          regex: (_userAgentRegex$exec2 = userAgentRegex.exec(userAgent)) === null || _userAgentRegex$exec2 === void 0 ? void 0 : _userAgentRegex$exec2.groups
        });
        return groups;
      } catch (_unused) {
        return null;
      }
    }

This is the output when it does reproduce (if printing from the jest file)

nicolo-ribaudo commented 3 years ago

It's still almost the same :thinking: https://www.diffchecker.com/AdAKHQcU

yoav-lavi commented 3 years ago

Why does removing a comment make it use var and _unused in the catch clause though? That's also a bit suspect

nicolo-ribaudo commented 3 years ago

It's probably because it trying to compile for different targets (older): it's using the transform-block-scoping and optional-catch-binding plugins. Maybe there is another Babel config somewhere?

nicolo-ribaudo commented 3 years ago

Try this: BABEL_ENV=test BABEL_SHOW_CONFIG_FOR=./src/index.ts yarn run jest

yoav-lavi commented 3 years ago
Babel configs on "$$PATH/babel-issue/src/index.ts" (ascending priority):
config $$PATH/babel-issue/babel.config.js
{
  "sourceType": "module",
  "presets": [
    "@babel/preset-typescript",
    "@babel/preset-react",
    "@babel/preset-env"
  ],
  "plugins": [
    [
      "@babel/plugin-transform-runtime",
      {}
    ],
    "lodash"
  ]
}

programmatic options from babel-jest
{
  "cwd": "$$PATH/babel-issue",
  "caller": {
    "name": "babel-jest",
    "supportsDynamicImport": false,
    "supportsExportNamespaceFrom": false,
    "supportsStaticESM": false,
    "supportsTopLevelAwait": false
  },
  "compact": false,
  "plugins": [],
  "presets": [
    "$$PATH/babel-issue/node_modules/babel-preset-jest/index.js"
  ],
  "sourceMaps": "both",
  "filename": "$$PATH/babel-issue/src/index.ts"
}

Babel configs on "$$PATH/babel-issue/src/index.ts" (ascending priority):
config $$PATH/babel-issue/babel.config.js
{
  "sourceType": "module",
  "presets": [
    "@babel/preset-typescript",
    "@babel/preset-react",
    "@babel/preset-env"
  ],
  "plugins": [
    [
      "@babel/plugin-transform-runtime",
      {}
    ],
    "lodash"
  ]
}

programmatic options from babel-jest
{
  "cwd": "$$PATH/babel-issue",
  "caller": {
    "name": "babel-jest",
    "supportsDynamicImport": false,
    "supportsExportNamespaceFrom": false,
    "supportsStaticESM": false,
    "supportsTopLevelAwait": false
  },
  "compact": false,
  "plugins": [],
  "presets": [
    "$$PATH/babel-issue/node_modules/babel-preset-jest/index.js"
  ],
  "sourceMaps": "both",
  "filename": "$$PATH/babel-issue/src/index.ts"
}
yoav-lavi commented 3 years ago

Looks identical with and without comments

yoav-lavi commented 3 years ago

@nicolo-ribaudo why would it try to change targets based on whether there was a comment in the source code though?

nicolo-ribaudo commented 3 years ago

No idea. One more thing: could you try adding

console.log((() => /(?<type>(Chrome|Firefox|Version|CriOS|FxiOS|SamsungBrowser))\/(?<version>\d+)(.|\s)/).toString())

in your TS file? I'm doing it, and it seems like the regexp is not being compiled.

yoav-lavi commented 3 years ago

That also causes it not to reproduce sadly

yoav-lavi commented 3 years ago

Since the output when it does reproduce is similar to what the function looks like when it's polyfilled for NodeJS 9, I think there may be two issues here:

  1. The RegExp groups polyfill doesn't work with the constructor / factory syntax
  2. Either babel or babel-jest will use a different target in some circumstances

This would theoretically explain why it is working in the cases where it is