Added cache object to node-sass-once-importer

[konpikwastaken]

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.

[coveralls]

Coverage remained the same at 100.0% when pulling 684439c on konpikwastaken:feature/file-cache into a7c7010 on maoberlehner:master.

[maoberlehner]

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!

[maoberlehner]

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.

[konpikwastaken]

I'll take a look at it!

[konpikwastaken]

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.

[maoberlehner]

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.

[konpikwastaken]

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!