remove lodash #1440
remove lodash #1440
Conversation
sdalonzo
force-pushed
the
no-mo-lodash
branch
from
955fcda to
69307a7
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1440 +/- ##
==========================================
- Coverage 94.46% 93.86% -0.60%
==========================================
Files 214 144 -70
Lines 9549 9434 -115
Branches 638 612 -26
==========================================
- Hits 9020 8855 -165
- Misses 515 562 +47
- Partials 14 17 +3 ☔ View full report in Codecov by Sentry. |
sdalonzo
force-pushed
the
no-mo-lodash
branch
5 times, most recently
from
50a6ef4 to
3877634
Compare
sdalonzo
force-pushed
the
no-mo-lodash
branch
4 times, most recently
from
87ab964 to
dbdb2b1
Compare
sdalonzo
force-pushed
the
no-mo-lodash
branch
from
dbdb2b1 to
2b1c4f9
Compare
| const update = (path) => { | ||
| const Node = path.value | ||
|
|
||
| // Dumb way to skip text elements since they also can have an align prop | ||
| if (get(Node, 'openingElement.name.name', '').toLowerCase().includes('text')) { | ||
| if (Node?.openingElement?.name?.name?.toLowerCase()?.includes('text')) { |
There was a problem hiding this comment.
(super-n-b) If that last name returns, toLowerCase() should work, no?
| if (Node?.openingElement?.name?.name?.toLowerCase()?.includes('text')) { | |
| if (Node?.openingElement?.name?.name?.toLowerCase().includes('text')) { |
| return string | ||
| .replace(/([a-z])([A-Z])/g, '$1-$2') | ||
| .replace(/[\s_]+/g, '-') | ||
| .toLowerCase() |
There was a problem hiding this comment.
This function doesn't return exactly the same results as lodash's version, but I'm not sure how true to the original we need to be. The most obvious difference is that hyphens can end up leading or trailing characters -- is it worth an enhancement to trim that?
There was a problem hiding this comment.
good catch! it's a naive copy from the internet, first goal was to get it building and measure the bundle change
| } from './constants' | ||
| import debounce from 'lodash.debounce' |
There was a problem hiding this comment.
(n-b) Kind of amazing that this was the only usage in DS
| export function debounce(func, wait, immediate) { | ||
| let timeout | ||
| return function () { | ||
| const context = this |
There was a problem hiding this comment.
(n-b) At least you didn't call it that 🤣
|
|
||
| export function debounce(func, wait, immediate) { | ||
| let timeout | ||
| return function () { |
There was a problem hiding this comment.
Don't you need to spread the arguments into this function? I had assumed that this creates a new scope for the args setting below and comes up empty.
| import * as icons from './index' | ||
|
|
||
| function upperFirst(string) { |
There was a problem hiding this comment.
(n-b) You've written this function twice -- the other instance is in utils
It's so small that repetition is probably fine, but do you want to consolidate?
| @@ -21,7 +21,7 @@ | |||
| * Specify a SemVer range to ensure developers use a Node.js version that is appropriate | |||
| * for your repo. | |||
| */ | |||
| "nodeSupportedVersionRange": ">=16", | |||
| "nodeSupportedVersionRange": ">=18", | |||
There was a problem hiding this comment.
(n-b) This get missed in the v6 pull?
| @@ -33,7 +33,6 @@ | |||
| "@babel/core": "^7.22.15", | |||
| "@priceline/babel-preset": "workspace:*", | |||
| "@priceline/eslint-config": "workspace:*", | |||
| "@reach/component-component": "^0.16.0", | |||
There was a problem hiding this comment.
(n-b) Oh, missed this one -- unused?
No description provided.