-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
chore: remove datetime package dependencies to lodash #6751
chore: remove datetime package dependencies to lodash #6751
Conversation
Generate changelog in
|
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.
Thanks! I agree with removing the dep on lodash
, especially since it was so light.
@@ -69,7 +68,7 @@ export function getDateObjectFromIsoString( | |||
if (value === undefined) { | |||
return undefined; | |||
} | |||
if (value === null || isEmpty(value)) { | |||
if (value === null || 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.
if (value === null || value === "") { | |
if (value === null || value === "" || typeof value !== "string") { |
Can you add a typeof
check here to be extra defensive in the event a non-TypeScript consumer is passing in an incorrect value here (such as a number) and expecting to get null
returned?
isEmpty
evaluates to true
on numbers, so I'd like to preserve that behavior to avoid an accidental break.
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.
Seems reasonable. Interesting that here lodash.isEmpty very much doesn't do what it says on the tin:
Checks if
value
is an empty object, collection, map, or set.
...which a number clearly isn't - handling for primitives isn't specified.
Checklist
Changes proposed in this pull request:
I noticed that
@blueprintjs/datetime
depends onlodash
for two extremely trivial use cases. I've changed those as such:getCurrentTimezone
, a function without arguments, usedmemoize
.I've replaced it with a global variable - while this is a new global
let
, the previous one was a mutable object stored in aconst
, so it's essentially the same.The function was also declared as
() => string
, meaning there was no official way to access its lodash cache property either.timezoneUtils
usedisEmpty
on a variable declared asstring | null | undefined
, out of whichnull
andundefined
were already checked for. I replaced this with a simple manual checkAdditionally,
@blueprintjs/datetime2
depended on@types/lodash
without using it for anything, so I've removed it as well.This effectively means you can build a Blueprint app without depending transitively on lodash.
Reviewers should focus on:
Whether there are edge cases that might break from these changes.