Replace lodash with es-toolkit #197
Conversation
myarmolinsky
requested review from
Page- and
thgreasi
and removed request for
Page-
myarmolinsky
force-pushed
the
replace-lodash-with-es-toolkit
branch
5 times, most recently
from
f0a460d to
c24e394
Compare
Change-type: patch
myarmolinsky
force-pushed
the
replace-lodash-with-es-toolkit
branch
from
c24e394 to
5c9ed4e
Compare
| const isEmptyDuration = Object.keys(rest).length === 0; | ||
|
|
||
| if (isEmptyDuration) { |
There was a problem hiding this comment.
There's no need to assign to an intermediary here:
| const isEmptyDuration = Object.keys(rest).length === 0; | |
| if (isEmptyDuration) { | |
| if (Object.keys(rest).length === 0) { |
| // Destructure 'negative' out, collect the 'rest' into a new object | ||
| const { negative, ...rest } = duration; | ||
|
|
||
| // Check if 'rest' is empty |
There was a problem hiding this comment.
This comment is kind of redundant, it tells me what the code is doing but I can see that by simply reading the code and it doesn't add anything - if it were to say why then that could be useful but its current form is just redundant
|
|
||
| const duration = omitBy(picked, isNil); | ||
|
|
||
| // Destructure 'negative' out, collect the 'rest' into a new object |
There was a problem hiding this comment.
Again this comment is also kind of redundant as it only tells me what the code already tells me
| .map((alias) => alias.toLowerCase()) | ||
| .value(); | ||
| // 1. Safely get your source arrays (using ?? [] to handle nulls like Lodash would) | ||
| const rels = getRelationships(clientModel.relationships) ?? []; |
There was a problem hiding this comment.
This always returns an array already so the ?? [] is completely pointless? It looks to me like this was written by an LLM and it's added a bunch of unnecessary parts
| import { range } from 'es-toolkit'; | ||
|
|
||
| const filterString = _.range(1, 2000) | ||
| const filterString = range(1, 2000) |
There was a problem hiding this comment.
This can also be done with native via:
| const filterString = range(1, 2000) | |
| const filterString = Array.from({ length: 2000 }, (_v, i) => 'id eq ' + i) |
(and also deleting the .mapvline, but github didn't let me target that part with this comment)
| operandToAbstractSQLFactory = (binds, resource) => { | ||
| // Case 1: Called with 1 argument -> (binds) | ||
| // partialRight behavior: fn(binds, 'team') | ||
| if (resource === undefined) { | ||
| return originalFactory(binds, 'team'); | ||
| } | ||
|
|
||
| // Case 2: Called with 2 arguments -> (binds, resource) | ||
| // partialRight behavior: fn(binds, resource, 'team') | ||
| return originalFactory(binds, resource, 'team'); | ||
| }; |
There was a problem hiding this comment.
I assume this was also written by an LLM, I think the more accurate equivalent would be:
operandToAbstractSQLFactory = (...args) => originalFactory(...args, 'team');There was a problem hiding this comment.
I had tried as you suggested originally, but then it complains that it expected 1-3 arguments but received 4 ('team' being the 4th)
| import test from './test.js'; | ||
|
|
||
| const createAggregate = function (args) { | ||
| const createAggregate = function (args: any) { |
There was a problem hiding this comment.
I'm not sure on why this change was made when nothing seems to have prompted it to be necessary? And the same for a bunch of other places that add an any without any corresponding change that would appear to make it now necessary
There was a problem hiding this comment.
All of those were complaining for me locally about being implicitly any instead of explicitly so I just made them explicit to clear the complaints
Change-type: patch
Fibery: https://balena.fibery.io/Work/Project/We-should-migrate-from-lodash-to-es-toolkit-(UI-CLI-etc.)-1895