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
Import per-function lodash modules #11789
Changes from 2 commits
ae5feee
3f65485
be7a4ad
040654c
1453bec
98edd64
e64348a
d0067a5
e50e41c
814a632
46d741b
4b5d099
539f2cd
d943cfa
35a0768
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
// @flow | ||
import path from "path"; | ||
import escapeRegExp from "lodash/escapeRegExp"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be possible to replace There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we instead consider replacing it with escape-string-regexp. That module is well tested, not being deprecated and gets 19 million weekly downloads (it's not below scrutiny). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keep in mind it requires major bump There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah sorry for missing that, I guess this suggestion would only apply to babel 8 then. |
||
import escapeRegExp from "lodash.escaperegexp"; | ||
|
||
const sep = `\\${path.sep}`; | ||
const endSep = `(?:${sep}|$)`; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,6 @@ | |
"dependencies": { | ||
"@babel/helper-function-name": "^7.10.4", | ||
"@babel/types": "^7.10.4", | ||
"lodash": "^4.17.13" | ||
"lodash.has": "^4.5.2" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That said, the YDNLU native JavaScript replacement is relatively complex (9 lines of non-trivial code including regular expressions) and generates an error when key paths do not exist. On balance it may make sense to maintain readability for now, although that's just my two cents and I'd welcome feedback. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's keep it, since we are using it with a deep path. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,6 @@ | |
}, | ||
"main": "lib/index.js", | ||
"dependencies": { | ||
"lodash": "^4.17.13" | ||
"lodash.pull": "^4.1.0" | ||
} | ||
} |
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.
We should be able to remove
defaults
, yeah?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.
Possibly! - we'd want to be careful about the argument ordering to the spread operator:
Also a subtle difference is that
lodash.defaults
mutates the first argument and returns it as a value, whereas the spread operator returns a new shallow clone; that's probably fine and safe in most cases, but worth being careful about.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.
In our case the mutation isn't a problem, since the first object is always defined inline in the call.
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.
Okie doke, thanks - #11797 contains an attempt to clean up the use of
lodash.defaults
from the codebase (including this callsite).