Closed konpikwastaken closed 6 years ago
Hey @konpikwastaken thanks for your pull request! I highly appreciate it!
I'll add a few comments for some very minor (style) improvements.
Big Thanks for contributing!
Also, some tests are failing, please take a look at it :)
You can run the tests locally with npm test
. Keep in mind tough, that you have to run npm run build
before, so that the integration tests actually test on the newest changes.
Thx.
I'll take a look at it!
Looks like tests pass now. After further review of the code - I removed one of the checks. The larger pull seems to be in glob.sync
call. Tests failed earlier too by the way, so I just assumed there was some issue and didn't look closely enough (looks like it has to do with your baseline file line endings perhaps?) Anyway, hope this small change helps others.
Hey @konpikwastaken I was already in the process of making some final touches and merging the branch into master, when I realized, that this is not the correct way of doing it.
The return of resolveUrl()
does depend on the url
and the includePaths
so you can't use only the url
for the cache.
const url = 'foo.scss';
const includePathsA = ['/path/foo', '/path/bar'];
const a = resolveUrl(url, includePathsA);
const includePathsB = ['/path/baz'];
const b = resolveUrl(url, includePathsB);
console.log(a === b); // false
You have to do something like that:
// Generates something like `/foo.scss|/path/foo|/path/baz`.
const cacheKey = [url, ...includePaths].join('|');
if (!cache.has(cacheKey)) {
cache.set(cacheKey, resolveUrl(
url,
includePaths,
));
}
resolvedUrl = cache.get(cacheKey);
Because the includePaths
contain the path to the file in which the @import
statement is, the cacheKey
will be different for every .scss
file, so I guess the benefits of using a cache would be very minor most of the time.
I've tested the performance improvements with Bootstrap, and they were basically non existent. Feel free to make your own tests tough. I'm happy to accept a pull request which uses the includePaths
for generating the cacheKey
if there are still measurable performance improvements in some cases.
Or maybe there is some other way of how to avoid glob.sync
? I'm open for suggestions and happy to help out if you have an idea.
Thx.
Hm, yes, I see what you're saying. I wonder if the improvement observed in our code base is due to the way that our imports are structured. I'll take another look once I get a chance.
As for the glob call, I'll think more on it :).
Cheers, sorry for the churn!
We have a very long build step, lots of files being written with a legacy & new code base.
This change speeds up file resolution for us by up to 15s on a baseline 75-80s build step.