-
Notifications
You must be signed in to change notification settings - Fork 260
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 preserveDirectories
option
#353
Conversation
removed |
86ae9a1
to
8c9e24f
Compare
8c9e24f
to
bfd7c54
Compare
@gijsroge However, I encountered a bug that would not preserve directory structure for the first subdirectory. Script:
Files/ directories:
All files in Is this something you can reproduce? |
I can't reproduce the skipping of the first directory as reported by @rolbr. For me it works great. I can say that when I was on the |
ty save my life <3 @gijsroge |
index.js
Outdated
@@ -33,6 +33,10 @@ const handleFile = async (sourcePath, {destination, plugins = []}) => { | |||
return returnValue; | |||
} | |||
|
|||
if (preserveDirectories) { | |||
returnValue.destinationPath = path.join(path.dirname(destinationPath), sourcePath.replace(baseDirectory, '')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think using .replace
will work cross-platform. Use the path
related methods for path manipulation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sourcepath is just a string generated from "globby" where I need to remove the baseDirectory
from. I can't find anything path
related to do this. So I assume this is just fine, if not, could you point me in the right direction?
index.js
Outdated
@@ -45,13 +49,14 @@ module.exports = async (input, {glob = true, ...options} = {}) => { | |||
} | |||
|
|||
const filePaths = glob ? await globby(input, {onlyFiles: true}) : input; | |||
const baseDirectory = path.dirname(filePaths[0]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the first file path decide the base directory? This should be documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
preserveDirectories
option
What is the status with this PR? Can we have it merged soon? |
@yellow1912 i'm picking this back up in the next few days. |
Getting the build script to handle this was more trouble than it should have been. `imagemin` has a 4 year old merge request[1] to handle recursing into subdirectories... As a work around when images are batched, the script loops over all the files in the image dir, running them in parallel groups of 4 (to speed things up, but not lock up the CPU). 1: imagemin/imagemin#353
Hey, just asking..What's the status of this PR? |
Any news about PR? |
This feature is essential 🎉 I use this command while waiting:
Thanks for imagemin 👍 |
I've implemented something similar locally. Concerning @sindresorhus's question on "Why does the first file path decide the base directory? This should be documented.", how about we only enable let destinationPath = destination ? path.join(destination, preserveDirectories? sourcePath: path.basename(sourcePath)) : undefined; |
Any movement? |
* rebase with upstream * add clarification what basePath does * make basePath configurable
bfd7c54
to
b3ce79c
Compare
So yeah, days turned into weeks, weeks turned into months, months turned into years. Sorry about the delay :( I addressed most of the issues + rebased with main, care to re-review? cc/ @1000ch @sindresorhus |
There is no solid way to detect if someone is using a glob or not even if |
@@ -35,6 +35,10 @@ const handleFile = async (sourcePath, {destination, plugins = []}) => { | |||
return returnValue; | |||
} | |||
|
|||
if (preserveDirectories) { | |||
returnValue.destinationPath = path.join(destination, path.parse(sourcePath).dir.replace(basePath, ''), path.basename(destinationPath)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use path.dirname
preserveDirectories: true, | ||
}); | ||
``` | ||
- `assets/images/1.jpg` -> `dist/1.jpg` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- `assets/images/1.jpg` -> `dist/1.jpg` | |
- `assets/images/1.jpg` → `dist/1.jpg` |
Tests are failing on Windows. |
@gijsroge thank you for that very important feature! Hope it will be merged soon! |
Very important feature! Hope it will be merged soon! |
Any updates about it? |
Very important feature! Hope it will be merged soon!!! |
is this not being worked on anymore? |
Any progress? This is really necessary. |
@Shayan-To @eBaeza, @rtatarinov, etc Please refrain from making such comments. There are plenty of people subscribed to threads and this kind of comment just spams their inbox. Please only comment if you have something to add to the discussion. If this is so "necessary" feel free to contribute with valuable help. This might even include making a PR to the fork, helping the original author. Thank you. |
Any progress on this? |
Closing for lack of activity from OP. |
Fixes #191
If the
preserveDirectories
is set the output will match the directory structure from the input files.Another possibility would be to introduce a breaking change and make this the default behaviour as that's what I would expect, but maybe i'm biased as I need this feature in a current project 😄
Given:
assets/images/1.jpg, assets/images/nested/2.svg, assets/images/nested/nested/3.webp
It will detect that
assets/images
is the basePath and will not copy over these directories to the destination.Our output would be
dest/1.jpg, dest/nested/assets/2.svg, dest/nested/nested/3.webp
cc/ #293 #262 #225 #213 #196 #192