Replace lodash 'isRegExp' usage with NodeJS util isRegExp (or a fallback when unavailable)

[jayaddison]

Q	A
Patch: Bug Fix?	No
Major: Breaking Change?	No
Minor: New Feature?	No
Tests Added + Pass?	No
Any Dependency Changes?	Yes
License	MIT

Similar to #11855, this changeset replaces a further lodash dependency (in this case, isRegExp) with a standalone alternative (the is-regexp library function).

Edit: also for context, adding a reference to #11789 (this is one of a collection of pull requests relating to reducing dependency upon lodash)

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit c4c2850:

Sandbox	Source
babel-repl-custom-plugin	Configuration
babel-plugin-multi-config	Configuration

[babel-bot]

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

[JLHwung]

_.isRegExp will call optimized node.js builtin util.isRegExp before toString comparison. Given that most Babel users are using node.js, I would suggest in this case we create our own isRegExp helper.

function isRegExp(value) {
  try {
    return require("util").isRegExp(value);
  } catch {
    return Object.prototype.toString.call(value) === '[object RegExp]';
  }
}

[JLHwung]

Oh util.isRegExp is deprecated, maybe we should target this change to Babel 8 via https://nodejs.org/api/util.html#util_util_types_isregexp_value.

[jayaddison]

Is it worth somehow pushing the require operation to a higher (module?) scope so that the try...require is only evaluated once?

[jayaddison]

Ah, I missed your second comment before replying. Yep, I'll retarget this against Babel 8 when applying a change to use a built-in NodeJS regexp test.

[JLHwung]

Is it worth somehow pushing the require operation to a higher (module?) scope so that the try...require is only evaluated once?

Good idea, we can even get rid of try catch here

var nodeUtilTypes = (function() {
  try {
    return process.binding('util').types;
  } catch {}
}());

isRegExp = value =>
   nodeUtilTypes ? nodeUtilTypes.isRegExp(value) : Object.prototype.toString.call(value) === '[object RegExp]';

[jayaddison]

Hrm. After a little more time to pause I'm less certain what to do here. The process.binding pattern doesn't appear elsewhere in the codebase, and although nodejs may be relatively well-adopted by babel users, I don't see too many references to platform-specific code (just one or two mentions of nodejs-related workarounds).

I'm not averse to applying those changes if they're definitely the way to go, but I'd prefer to understand a bit more about the options and implications first.

[coreyfarrell]

Note that process.binding is also deprecated (see DEP0103 and DEP0111). For using Node.js methods I would probably go for:

const nodeUtilTypes = util.types || util;

This way you would use util.types.isRegExp in Node.js 10+, util.isRegExp in older versions. Although util.isRegExp is deprecated in Node.js 4+ it's never going to be removed or produce a warning in Node.js < 10 so it seems like a reasonable fallback.

[JLHwung]

As of process.binding(), I meant to avoid interactions with bundlers -- that require("utils") is intended to be failed on browsers. Since we have offered a fallback, we don't expect the bundlers do anything about require("utils") So users don't have to provide node: { utils: empty } to Webpack or use rollup-plugin-node-polyfills that may even try to polyfill utils.isRegExp.

As a note, @babel/standalone bundles @babel/types and it is run in browsers. Building @babel/standalone does not fail because we have rollup-plugin-node-polyfills, but I don't expect @babel/types bundling users have to change their bundler config in a non-breaking releases.

[jayaddison]

@JLHwung @coreyfarrell The code structure has been refactored a bunch since the previous comments in this discussion; in particular the top-level util module is always imported, and then a local isRegExp helper is assigned based on a series of preferentially-ordered fallbacks. Hopefully that meets most of the optimization goals mentioned while also catering for older Node versions; glad for further feedback.

[kaicataldo]

Any reason we can't just use lodash.isregexp instead? I don't think we need to get rid of lodash entirely, but it would be great if we could just install the utils we're using individually.

[TrySound]

lodash/lodash#3838

[AmyShackles]

Probably a stupid question, but why not just test for the constructor?

let reg = /literal/gi
let reg2 = new RegExp(/constructorWithRegexPattern/, "gi");
let reg3 = new RegExp("constructorWithString", "gi");
let reg4 = RegExp(/factoryNotationWithRegexPattern/, "gi");
let reg5 = RegExp("factoryNotationWithString", "gi");
console.log(reg.constructor.name) // "RegExp"
console.log(reg2.constructor.name) // "RegExp"
console.log(reg3.constructor.name) // "RegExp"
console.log(reg4.constructor.name) // "RegExp"
console.log(reg5.constructor.name) // "RegExp"

It seems like you would just need to test for undefined/null and then check if the constructor was equivalent to RegExp. Then you wouldn't need to bring in a util or use lodash (for this particular piece of code).

if (value && value.constructor === RegExp) {...}

[JLHwung]

@AmyShackles The constructor property can be overwritten for non-primitive values.

const foo = {};
foo.constructor = RegExp;
foo.constructor === RegExp // returns true

[jayaddison]

(closing and cleaning up a few stale pull requests; see #11789 (comment) and subsequent comments in case it's useful to re-open from github pr refs at a later date)