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

Fix: Cache path #9

Closed

Conversation

naranjamecanica
Copy link
Contributor

Use pkg-dir to find projects base directory and us path.join to create crossplatform paths.

Use pkg-dir to find projects base directory and us path.join to create crossplatform paths.
@vHeemstra
Copy link
Owner

vHeemstra commented Nov 20, 2023

Thanks for the PR!

Some adjustments:

  • Since all paths are normalized using Vite's normalizePath() (so using forward slashes), there's no need for path.join(). Moreover, path.join on Windows would actually insert backward slashes, so I rather not use that to keep consistent (cross-platform) paths.
  • Instead of added another package, I would rather add the actual function that gets the package directory. Also, earlier, I tried adding this package, but there was some conflict on the engine/Node versions. pkg-dir only supports Node 18+ (in its package config only).
  • Also, I would like to add the option to choose the cacheDir:
    • (default) <packageRoot>/node_modules/.cache/vite-plugin-imagemin/
    • relative path string, like some-dir, would become <root>/some-dir/
    • absolute path string
  • Maybe add the option clearCache, so users can make their own logic in their config for clearing the cache.

I will create a new branch with these functionalities. Any additional suggestions can be PR'd there.

https://github.com/vHeemstra/vite-plugin-imagemin/tree/improve-cache-handling

@vHeemstra
Copy link
Owner

I made a new branch with all the changes mentioned above and created a new PR: #10

Can you have a look? If you have any suggestions/changes/etc, please feel free to comment there. (Or create a PR to that branch.)

Otherwise, I will merge the branch somewhere this week.

@vHeemstra vHeemstra closed this Nov 20, 2023
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