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

feat: add support for target path transform #284

Merged

Conversation

Fetz
Copy link
Contributor

@Fetz Fetz commented Aug 28, 2018

updated version of the PR: /pull/115.

Should resolve #107.

Why another PR?

Since the other PR is not being updated for some time, and needed to be updated for some time, since I did some differences (mostly to allow Promises with transformPath will need a review)

differences:

  • added support to Promises to be aligned with transform (since transform supports Promises)
  • updated the tests since there are more test files test files
  • updated the Readme to include implementation examples (Function & Promise) like transform in the Readme
  • rebased with the latest version

@jsf-clabot
Copy link

jsf-clabot commented Aug 28, 2018

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

@EdBoucher
Copy link

Any updates on getting this into master? It's extremely useful.

@alexander-akait
Copy link
Member

Please rebase PR

@alexander-akait
Copy link
Member

@Fetz can you describe use case? Maybe better use to as function?

@Fetz
Copy link
Contributor Author

Fetz commented Oct 7, 2018

Rebase done

@Fetz
Copy link
Contributor Author

Fetz commented Oct 7, 2018

Hi @evilebottnawi 👋,

My use case:

  • I wanted to copy file from a folder to a dist file, but I didn't want part of the path of the file to be included
  • I was already using webpack, and I didn't want to add another tool (gulp, grunt or some shell script)

Why I used transformPath and not to?

Since there was already a PR open with the functionality and there was ongoing comments with it, and by the comments, I assume that the PR was only blocked, because he need to rebase and update the documentation, and since the original creator, wasn't active working anymore on it I decided to pick his work where he left.

Why makes more sense (for me) to use transformPath instead of to

  • We have already transform logic, so a transformPath is consistent with the current options interface
  • I assume that most of the times, that the users target folder will always be the same but most of the times users will need to transform how the path will be generated inside the target folder
  • Will be more readable where the files would be copy to (top level of the file tree), and if the user needs more detail of the rest of the path, he can read the transformPath function

as well the original author of the PR included is own take on maybe why should be better:

Thinking a bit about this, the only benefit of having both templates and a transform is that templates support content hashing. Everything else should be fairly easy to implement in the transform function, if needed.
No idea how often this hash option is used and if it is worth introducing a special transformPath option because of it.
@patkec #115 (comment)

Other's people use case

Since I can't talk for everyone, but other people uses case can be found here:

@Fetz
Copy link
Contributor Author

Fetz commented Oct 11, 2018

Rebase done

@alexander-akait
Copy link
Member

/cc @michael-ciniawsky

@Fetz
Copy link
Contributor Author

Fetz commented Oct 20, 2018

Rebase done

@alexander-akait
Copy link
Member

/cc @michael-ciniawsky

@Fetz
Copy link
Contributor Author

Fetz commented Oct 24, 2018

@evilebottnawi, maybe @michael-ciniawsky is "ignoring" this PR because is missing the label?
Or there is some higher priority (or complexity related), that other PR's are getting priority?

Is a bit weird that other PR, are being created and being merged faster than this one

@alexander-akait
Copy link
Member

@Fetz let's wait 24 hours and merge if not answered, feel free ping me

@jomaf1010
Copy link

No choice but to fork from @Fetz , still watching for this PR tho

@Fetz
Copy link
Contributor Author

Fetz commented Oct 25, 2018

👋 @evilebottnawi pinging you as a reminder regarding the 24 hours time

@alexander-akait
Copy link
Member

@Fetz let's merge this release will be after resolve #302 (looks seriously bug)

@alexander-akait alexander-akait merged commit 7fe0c06 into webpack-contrib:master Oct 25, 2018
@Fetz
Copy link
Contributor Author

Fetz commented Oct 25, 2018

@evilebottnawi, thanks a lot for merging this

@Fetz
Copy link
Contributor Author

Fetz commented Nov 2, 2018

@jomaf1010 you can now use the copy-webpack-plugin@4.6.0, that includes this feature

@EdBoucher
Copy link

Brilliant, thanks guys!

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.

[Feature] Manually modify output path
5 participants