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

Caching is incompatible with @imports #15

Closed
cvn opened this issue Feb 24, 2014 · 8 comments · Fixed by #17
Closed

Caching is incompatible with @imports #15

cvn opened this issue Feb 24, 2014 · 8 comments · Fixed by #17
Labels

Comments

@cvn
Copy link

cvn commented Feb 24, 2014

When cache: true is set and imports: [] is defined, grunt-lesslint ignores changes to @imported files. I tested this by inserting a linting error into an imported less file. Unless I change the parent file, this error is not caught during linting.

@jgable
Copy link
Owner

jgable commented Feb 24, 2014

Hmm... are your imported files not run through less lint as well? Wouldn't they fail upstream or downstream?

@cvn
Copy link
Author

cvn commented Feb 24, 2014

Our architecture is that we have one main less files with a bunch of partials that get imported. It's not practical to lint the partials themselves because they use variables from a separate partial, and cause false positives about missing variables.

So we only lint the main file, and add the partials directory to the imports: [] config. Without caching, this works very well, including giving us accurate line numbers for linting errors in the partials. With caching on, it will ignore changes in the partials if there is no change in the main less file, and will miss any linting errors that are introduced after the cache is primed.

@jgable jgable added the bug label Feb 24, 2014
@jgable
Copy link
Owner

jgable commented Feb 24, 2014

So, essentially we'd have to change the cache key of the file from just that files contents to its contents and any imports contents. I'll investigate and see what the impact is on speed.

@jgable
Copy link
Owner

jgable commented Feb 26, 2014

This is proving to be tricky, I can build a naive solution that just parses the less into css every time and then uses the css as a cache key but that doesn't really improve performance as much as now because the less parsing is what we're trying to avoid.

Ideally, I think we'd do a regex match for import statements, grab their file contents (keep the file contents around for later lookups from other less that import it), add all the contents together as hash key.

The problems I foresee with this are relative path resolution vs passing in less options with import lookup paths.

I'm going to punt for now and probably push up something that parses every time since there is a risk of false positive cache hits right now.

@cvn
Copy link
Author

cvn commented Feb 26, 2014

Your idea sounds good, but I can imagine how it would be tricky. To start with, you'd have to recursively regex match all @import'ed less files for more @imports to build a complete set for hashing, right?

What about this: Read the less imports: [] option and hash the complete result set. Then compare that hash every time. It wouldn't have awareness of what files were used by the primary less files, and as a result would invalidate the cache when unrelated files changed, but it would eliminate false positive cache hits and still make things quicker for people doing most of their work in their primary less files instead of in their partials. This would only be necessary when the imports: [] array is non-empty, of course.

@jgable
Copy link
Owner

jgable commented Feb 27, 2014

@cvn just to update on this, plan to go with your strategy of hashing the imports: [] (and possibly caching the hash based on all the names of the import globbed files). Don't really have an ETA but if I can find time today I'll do it.

@jgable
Copy link
Owner

jgable commented Feb 27, 2014

The intermediate fix has landed in 0.19.0.

@cvn
Copy link
Author

cvn commented Feb 27, 2014

Very cool. Thanks, @jgable

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

Successfully merging a pull request may close this issue.

2 participants