Drop lodash #89
Drop lodash #89
Conversation
SukkaW
force-pushed
the
drop-lodash
branch
2 times, most recently
from
9f23922 to
033e75c
Compare
![windsurf-bot[bot]](https://avatars.githubusercontent.com/in/1066231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Other comments (4)
- src/transformations/compress.js (73-73) The change from `_.isEmpty(props.options)` to `props.options.length === 0` assumes that `props.options` is always an array. If `props.options` could be undefined, null, or an object in some cases, this change might introduce bugs. Consider adding a check to ensure `props.options` is an array or has a length property before accessing it.
- src/transformations/validate.js (91-91) The replacement of `_.isEmpty(props.hostnames)` with `!props.hostnames?.length` is close but not exactly equivalent. While it handles null/undefined cases correctly with the optional chaining, an empty array would evaluate to `length === 0` which is falsy. However, if you want to be more explicit, you could use `!props.hostnames || props.hostnames.length === 0`.
-
src/index.js (57-57)
For consistency with the other property checks, consider using optional chaining here as well:
if (source.name?.length > 0) { -
src/index.js (57-57)
For consistency with how other properties are checked, consider simplifying this to match the pattern used elsewhere:
if (source.name?.length) {
💡 To request another review, post a new comment with "/windsurf-review".
| if (_.startsWith(toTest, '://')) { | ||
| toTest = _.trimStart(toTest, '://'); | ||
| if (toTest.startsWith('://')) { | ||
| toTest = toTest.replace('://', ''); |
There was a problem hiding this comment.
The replacement of _.trimStart(toTest, '://') with toTest.replace('://', '') might not be equivalent in all cases. trimStart removes all leading occurrences of the specified characters, while replace only replaces the first occurrence. Consider using toTest.substring(3) instead for a more direct replacement.
There was a problem hiding this comment.
This is equivalent. We already know that :// is always at the start (that's what .startsWith(://) for). So, it will always replace the leading one.
Also, unlike .slice(3), .replace() prevents string CoW inside Node.js v8, which helps release memory pressure by allowing old strings to be GCed from the memory (.slice() holds the reference of the original string, preventing that from being GCed).
| for (let i = filtered.length - 1; i >= 0; i -= 1) { | ||
| const isValidRule = valid(filtered[i], this.allowedIP); | ||
| const isCommentOrEmptyLine = ruleUtils.isComment(filtered[i]) || _.isEmpty(filtered[i]); | ||
| const isCommentOrEmptyLine = ruleUtils.isComment(filtered[i]) || filtered[i].length === 0; |
There was a problem hiding this comment.
The replacement of _.isEmpty(filtered[i]) with filtered[i].length === 0 might not handle undefined or null values correctly. Consider using !filtered[i] || filtered[i].length === 0 to maintain the same behavior as lodash's isEmpty.
There was a problem hiding this comment.
This is not necessary. We already know that the filtered is string[], so filtered[i] will never be nullish.
| */ | ||
| function valid(ruleText, allowedIP) { | ||
| if (ruleUtils.isComment(ruleText) || _.isEmpty(_.trim(ruleText))) { | ||
| if (ruleUtils.isComment(ruleText) || ruleText.trim().length === 0) { |
There was a problem hiding this comment.
The replacement of _.isEmpty(_.trim(ruleText)) with ruleText.trim().length === 0 might throw an error if ruleText is undefined or null. Consider using a null check: !ruleText || ruleText.trim().length === 0.
There was a problem hiding this comment.
We already know that ruleText is a string and will never be falsy.
|
cc @jellizaveta Would you like to review this? |
1 participant
Drop lodash from
HostslistCompiler.WIP: some unit tests are failing, still figuring out why.All fixed.