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

add minimize cache directory option #40

Closed
wants to merge 6 commits into from
Closed

Conversation

e7h4n
Copy link
Contributor

@e7h4n e7h4n commented May 21, 2013

I added a cache for it because imagemin task is too slow.

Type: `String`
Default: `null`

Set image optimization cache directory, this directory will cache the result of [OptiPNG] and [jpegtran].
Copy link
Contributor

Choose a reason for hiding this comment

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

Those links have no definitions. :)

@passy
Copy link
Contributor

passy commented May 21, 2013

Very cool, caching the results is definitely a good idea.

@sindresorhus
Copy link
Member

I kinda wish grunt had this built-in, but I guess it's needed as this task can be slow.

Can you add a test to make sure it caches it?

And use os.tmpdir if available. (it's not available in node 0.8)

You also need to fix the merge conflict.

@e7h4n
Copy link
Contributor Author

e7h4n commented May 24, 2013

@sindresorhus Thanks, I'll write some test case. But I think system temp directory can be assigned in Gruntfile.js to keep task option concise like this:

imagemin: {
    option: {
        cache: path.resolve(os.tmpdir(), 'grunt-imagemin-cache')
    }
}

@sindresorhus
Copy link
Member

@perfectworks i don't see the point of having an option when we can handle it automatically.

@sindresorhus
Copy link
Member

ping

1 similar comment
@sindresorhus
Copy link
Member

ping

@e7h4n
Copy link
Contributor Author

e7h4n commented Jun 17, 2013

@sindresorhus I was in vacation last month, and will modify pull request this week.

@Antonio-Laguna
Copy link

ping? I wouldn't mind to do it but I came here every day to see if it's done 😄

@e7h4n
Copy link
Contributor Author

e7h4n commented Jul 1, 2013

Sorry for waiting, I've done testcase and use os.tmpdir to auto generate cache directory.


> This directory will cache the result of [OptiPNG](http://optipng.sourceforge.net) and [jpegtran](http://jpegclub.org/jpegtran/).


Copy link
Member

Choose a reason for hiding this comment

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

I don't see any reason this should be an option. Caching can happen transparently.

@sindresorhus
Copy link
Member

@perfectworks can you update regarding my comments and fix merge conflict? I want to merge this asap.

@sindresorhus
Copy link
Member

ping

@e7h4n
Copy link
Contributor Author

e7h4n commented Aug 6, 2013

ping

sindresorhus added a commit that referenced this pull request Aug 11, 2013
@sindresorhus
Copy link
Member

Thanks! Landed :)

🍰

@haggholm
Copy link

I've run into problems caused by this feature. Because it creates a dir under /tmp (well, os.tempdir, /tmp for me) with a deterministic name, this means that if multiple users on the same machine attempt to run imagemin, the path will clash. Since the permissions are 644 (again, for me -- this seems like a typical default), this means that the second user will get

Error: Unable to write "/tmp/grunt-contrib-imagemin.cache/9200eea54d49d30c49dcf749081098dddc844003" file (Error code: EACCES).

I suppose keeping the name consistent is a feature, not a bug (I'm guessing the point is to leave the temp files so they can be accessed as a cache), but then sharing a dirname between users becomes a problem. Perhaps the dir could include $USER?

(In my case I hit the problem because I run different sites under different users to separate security concerns. I can work around it by deleting the files from the corresponding accounts, or as root. It could be a major problem, though, for people building on shared hosting.)

@niftylettuce
Copy link

had the same issue

@sindresorhus
Copy link
Member

@haggholm yup, sounds reasonable.

@juriejan
Copy link

A bit late, but this plugin might allow you guys to move out the caching functionality from this plugin:

https://github.com/tschaub/grunt-newer

@francishogue
Copy link

Having the same kind of problems as @haggholm and @niftylettuce.

e7h4n added a commit to e7h4n/grunt-contrib-imagemin that referenced this pull request Jan 13, 2014


Minimized images will be cached to `os.tmpdir()` directory by default,
user can use environment variable `GRUNT_IMAGEMIN_CACHE` to specific
another directory to store cached images.
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.

None yet

8 participants