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.
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
Replace lodash 'isRegExp' usage with NodeJS util isRegExp (or a fallback when unavailable) #11856
Replace lodash 'isRegExp' usage with NodeJS util isRegExp (or a fallback when unavailable) #11856
Conversation
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:
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/32350/ |
@@ -1,6 +1,6 @@ | |||
// @flow | |||
import isPlainObject from "lodash/isPlainObject"; | |||
import isRegExp from "lodash/isRegExp"; | |||
import isRegExp from "is-regexp"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_.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]';
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth somehow pushing the require
operation to a higher (module?) scope so that the try...require
is only evaluated once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
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. |
… prototype comparison fallback
Probably a stupid question, but why not just test for the constructor?
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).
|
@AmyShackles The const foo = {};
foo.constructor = RegExp;
foo.constructor === RegExp // returns true |
Co-authored-by: ExE Boss <3889017+ExE-Boss@users.noreply.github.com>
(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) |
Similar to #11855, this changeset replaces a further
lodash
dependency (in this case,isRegExp
) with a standalone alternative (theis-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
)