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

use posix path as assets key #378

Closed
wants to merge 3 commits into from
Closed

Conversation

snadn
Copy link

@snadn snadn commented May 6, 2019

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

should use posix path as assets key that consistent with webpack

Breaking Changes

Additional Info

@alexander-akait
Copy link
Member

Why? What is problem?

@snadn
Copy link
Author

snadn commented May 6, 2019

  1. Consistent with webpack
    image

  2. Consistent with copy-webpack-plugin@4

  3. Consistent with option and can be used by other plugin friendly
    image

@alexander-akait
Copy link
Member

Don't use / in to, it is not compatobility with windows, use path.join/path.resolve

@snadn
Copy link
Author

snadn commented May 6, 2019

I think the key of assets should be treated as path of url instead of path of filesystem.

Regarding the third point, the value of 'to' may be 'js' (a dir), And my configuration in ‘tags’ is still expected to be ‘js/xxx.js’

And the first and second points are also important reasons, especially the first point.

@alexander-akait
Copy link
Member

@snadn no, linux support \ as part of name so in theory it can break cache

@snadn
Copy link
Author

snadn commented May 6, 2019

@evilebottnawi But this is just that this pr modification is not good enough, but using ‘/’ is still more reasonable. Maybe getting posix path and record it when getting the file path will be better.

ps: just change the key of assets will break cache? Whether it is in window or linux, the keys obtained are the same.The potential problem should be that multiple files may be mapped to a key

@alexander-akait
Copy link
Member

@snadn

  1. Use normalize-path instead posix
  2. Need tests

@codecov
Copy link

codecov bot commented May 8, 2019

Codecov Report

Merging #378 into master will decrease coverage by 0.28%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #378      +/-   ##
==========================================
- Coverage   94.82%   94.53%   -0.29%     
==========================================
  Files           9        9              
  Lines         290      293       +3     
  Branches       78       81       +3     
==========================================
+ Hits          275      277       +2     
- Misses         14       15       +1     
  Partials        1        1
Impacted Files Coverage Δ
src/postProcessPattern.js 88.75% <83.33%> (-0.87%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b61c86...09e2fa3. Read the comment docs.

@snadn
Copy link
Author

snadn commented May 23, 2019

@evilebottnawi Can this pr be merged?

@alexander-akait
Copy link
Member

Looks like related #383

@jeffposnick
Copy link

I was going to file a PR (see https://github.com/webpack-contrib/copy-webpack-plugin/compare/master...jeffposnick:windows-fix?expand=1) but I see that this PR should address it just as well.

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.

4 participants