-
Notifications
You must be signed in to change notification settings - Fork 252
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
refactor: drop lodash.trim
dependency
#749
Conversation
Use JavaScript's built-in String.prototype.trim method instead of lodash.trim, reducing dependency bloat. In one case, we were calling lodash.trim with a Buffer instance. lodash.trim calls toString on its argument, so we didn't notice this issue before. Continue calling toString before calling String.prototype.trim to preserve behavior.
@Kreeg this seems to work fine from my tests. Does it LGTY? |
lib/svg-sprite.js
Outdated
svg = trim(svg); | ||
name = trimStart(name.trim(), `${path.sep}.`) || path.basename(file); | ||
// TODO: Avoid Buffer -> String -> Buffer conversion of svg. | ||
svg = svg.toString().trim(); |
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.
Just wondering, would it make more sense to check if svg
is Buffer with Buffer.isBuffer()
and if so only then call toString()
?
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.
This would seem a little overengineering for me.
After a closer look, this is not identical to what we had before. Previously, if you passed
|
a2ee9ec
to
4082eeb
Compare
name = trimStart(name.trim(), `${path.sep}.`) || path.basename(file); | ||
// TODO: Avoid Buffer -> String -> Buffer conversion of svg. | ||
if (Buffer.isBuffer(svg)) { | ||
svg = svg.toString(); |
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.
Nah, I think that's OK.
Supersedes #742
Refs #598