Replace _.trim with native JS
#418
Conversation
| name = _.trimStart(_.trim(name), path.sep + '.') || path.basename(file); | ||
| svg = _.trim(svg); | ||
| name = _.trimStart(name.trim(), path.sep + '.') || path.basename(file); | ||
| svg = typeof svg === 'string' ? svg.trim() : svg; |
There was a problem hiding this comment.
@jkphl So, the question is do we need to trim here? svg can be a Buffer which resulted in failures without this check, but it makes me wonder.
There was a problem hiding this comment.
@XhmikosR Hmmm … I'm a bit unsure here (and cannot test in this very moment). It's possible that _.trim() has the neat side effect to automatically serialize Buffers and convert non-strings to empty strings, so that svg reliably is a string in the end. Can you confirm this? If so, we should ensure that svg ends up as a string in the end. Apart from that, we can surely use the native method.
There was a problem hiding this comment.
That I don't know TBH. It wasn't caught by the tests at all, I just hit it since I'm manually testing the master and lint branches on https://github.com/twbs/icons/blob/main/package.json#L28
There was a problem hiding this comment.
@jkphl yup, _.trim does cast the value to string before trimming.
So, the question is if we need to convert this to a string if it's not one? Or why even attempt to trim if it's a Buffer for instance. In my case svg is a Buffer and it's only used a few lines below for Vinyl contents. Which further makes me wonder if Buffer.from is needed there in case svg is already a Buffer?
Anyway, we can clean that up later, separately from this.
There was a problem hiding this comment.
I'm not absolutely sure about my original intentions here. I guess I wanted to allow both string and Buffer arguments (or possibly I didn't expect Buffer arguments at all in the beginning and they just happened to work?). Using _.trim() on the svg parameter makes sure we don't add invalid shapes consisting of, say, whitespace characters only. Not sure how easily one can ensure this for a Buffer as well. Casting a Buffer to a string and then re-creating as a Buffer definitely isn't necessary. But we would have to make sure that also string arguments still work as we don't know how people use the add() method. So maybe you're right and we should leave this as is for the time being and come back at a later point.
There was a problem hiding this comment.
There are also Buffer.isBuffer() or vinyl.isBuffer(). Either way, this is not currently well covered by the tests, so let's leave it for now and we'll revisit later.
|
@XhmikosR I think we can go back to this later. |
Second attempt after #408.