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.

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

Only call .toString() on sass files #47

Merged

Conversation

crazy2be
Copy link
Contributor

@crazy2be crazy2be commented Jan 2, 2019

Calling .toString() is a fairly expensive operation, which allocates
a new string, and copies all of the contents of the file Buffer into
that string. This is fairly fast for small files or small sites, but
quickly starts to take seconds of the total build time on medium
sized sites (in my case, roughly 2 seconds of the overall build was
this call to toString()!)

This change makes it so we only call toString() if we actually want
the result - i.e. if it as a sass file. Thus, we don't call it on
markdown files, image files, video files, etc.

Calling .toString() is a fairly expensive operation, which allocates
a new string, and copies all of the contents of the file Buffer into
that string. This is fairly fast for small files or small sites, but
quickly starts to take seconds of the total build time on medium
sized sites (in my case, roughly 2 seconds of the overall build was
this call to toString()!)

This change makes it so we only call toString() if we actually want
the result - i.e. if it as a sass file. Thus, we don't call it on
markdown files, image files, video files, etc.
].join(''));
} else if (err) {
error = new Error(err);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just indentation changes due to the inversion of the if statement.

@stevenschobert
Copy link
Owner

Hey @crazy2be! This is great, thanks for taking the time to put together a PR! I'll try and get this merged and released soon. I'll say sorry in advance for any delays on my part to get this deployed, my free time is really limited at the moment 😅

@stevenschobert stevenschobert merged commit d443be9 into stevenschobert:master Jan 20, 2019
@stevenschobert
Copy link
Owner

Thanks for the fix @crazy2be! Sorry for the delay.

This has been released in v1.6.0!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants