Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Replace _.trim with native JS #418

Closed
wants to merge 1 commit into from
Closed

Replace _.trim with native JS #418

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions lib/svg-sprite.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,15 +84,15 @@ SVGSpriter.prototype._namespacePow = [];
SVGSpriter.prototype.add = function(file, name, svg) {
// If no vinyl file object has been given
if (!this._isVinylFile(file)) {
file = _.trim(file);
file = file.trim();
let error = null;

// If the name part of the file path is absolute
if (name && path.isAbsolute(name)) {
error = format('SVGSpriter.add: "%s" is not a valid relative file name', name);
} else {
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;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

@XhmikosR XhmikosR Mar 7, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@XhmikosR XhmikosR Mar 11, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Argument validation
if (arguments.length < 3) {
Expand Down