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

Backslashes in asset paths in v5 (breaking change from v4) #383

Closed
jeffposnick opened this issue May 22, 2019 · 11 comments · Fixed by #392
Closed

Backslashes in asset paths in v5 (breaking change from v4) #383

jeffposnick opened this issue May 22, 2019 · 11 comments · Fixed by #392

Comments

@jeffposnick
Copy link

  • Operating System: Windows 10
  • Node Version: 10.15.3
  • NPM Version: 6.4.1
  • webpack Version: 4.32.1
  • copy-webpack-plugin Version: 5.0.3

Expected Behavior

The paths exposed in webpack for assets copied over on Windows-based systems using copy-webpack-plugin v5 should use forward slashes (/) as path separators.

Asset paths in the webpack output are meant to be usable as URLs, which always use / as a path separator.

Actual Behavior

Starting in copy-webpack-plugin v5, when performing a webpack build on a Windows-based system, backslashes (\) are used as the path separator in the final asset paths passed to webpack.

This creates downstream issues for other plugins that want to read the asset paths and treat them as URLs, as described in GoogleChrome/workbox#2057

This is a breaking change in behavior vs. copy-webpack-plugin v4.

Code

const path = require('path');
const CopyWebpackPlugin = require('copy-webpack-plugin');
 
module.exports = {
    mode: 'production',
    entry: {
        index: path.join(__dirname, 'src', 'js', 'index.js'),
    },
    output: {
        path: path.join(__dirname, 'dist'),
    },
    plugins: [
        new CopyWebpackPlugin([
            'src/**/*{css,html}'
        ])
    ]
};

How Do We Reproduce?

Use the following webpack config with a basic input filesystem—the actual contents don't really matter so much, and in this case I had a src/index.html and src/css/index.css. Compare the output using the v4 and v5 branches of copy-webpack-plugin.

In v4, the build process output looks like the following, using the "correct" / path separators:

Hash: 3223b6131435ca79accf
Version: webpack 4.32.1
Time: 346ms
Built at: 05/22/2019 1:10:22 PM
            Asset       Size  Chunks             Chunk Names
         index.js  947 bytes       0  [emitted]  index
src/css/index.css   34 bytes          [emitted]
   src/index.html   13 bytes          [emitted]
Entrypoint index = index.js
[0] ./src/js/index.js 18 bytes {0} [built]

In v5, the build process output looks like the following, using the "incorrect" \ path separators:

Hash: 3223b6131435ca79accf
Version: webpack 4.32.1
Time: 153ms
Built at: 05/22/2019 1:07:05 PM
            Asset       Size  Chunks             Chunk Names
         index.js  947 bytes       0  [emitted]  index
src\css\index.css   34 bytes          [emitted]
   src\index.html   13 bytes          [emitted]
Entrypoint index = index.js
[0] ./src/js/index.js 18 bytes {0} [built]
@alexander-akait
Copy link
Member

Looks like a bug, PR welcome

@jeffposnick
Copy link
Author

#378 looks like it should fix this.

@brian428
Copy link

I think #389 also fixes this, so the devs will probably need to look at both and see which approach they prefer.

@alexander-akait
Copy link
Member

alexander-akait commented Jul 3, 2019

/cc @jeffposnick not sure it is bug in copy-webpack-plugin

The paths exposed in webpack for assets copied over on Windows-based systems using copy-webpack-plugin v5 should use forward slashes (/) as path separators.

Disagree, assets path is path, not url, so i think we need normalize this on workbox plugin side.

Also using / in path on windows break watch mode for this plugin, because chokidar require use right path separator for watching

This is a breaking change in behavior vs. copy-webpack-plugin v4.

Yes, because in v4 watch doesn't work on windows, now it is works perfect

@jeffposnick
Copy link
Author

Disagree, assets path is path, not url, so i think we need normalize this on workbox plugin side.

That's surprising to hear (CC: @TheLarkInn). That assertion seems to be in conflict with the information from, e.g., the webpack docs for publicPath:

The value of the option is prefixed to every URL created by the runtime or loaders. Because of this the value of this option ends with / in most cases.

AFAIK, there's isn't a separate field in a webpack asset for filesystem path; each asset is uniquely named by what's effectively a relative URL, and copy-webpack-plugin now uses \ separators for those values on Windows. What happens if a Windows user uses copy-webpack-plugin with a publicPath of 'https://cdn.com/'? The final assets would end up assigned something like https://cdn.com/src\styles\index.css, and that doesn't make any sense.

This isn't just breaking Workbox's plugin; this is a change that appears to have led to angular/angular-cli#14593 (if not other issues as well), and at least two folks have opened PRs to go back to the previous behavior because they were also affected.

I appreciate the fact that the change was made in order to ensure that watch works on Windows, but hopefully either of those PRs that were filed could lead to both a functional watch as well as correct output paths.

@alexander-akait
Copy link
Member

@jeffposnick hm, yep, need investigate, will do this in near future, thanks for feedback

@bbortt
Copy link

bbortt commented Jul 22, 2019

Hi @evilebottnawi
May I ask when you're planning the next release? Because this bugfix is blocking us from an Angular8 upgrade... The commits to master since 5.0.3 don't look breaking, so a 5.0.4 patch maybe?
Thanks for your feedback!
BR, Timon

@alexander-akait
Copy link
Member

@bbortt today/tomorrow release

@bbortt
Copy link

bbortt commented Jul 26, 2019

@evilebottnawi friendly reminder to release 5.0.4 please.. I really don't want to pressure you but our upgrade pipeline (angular:^8.0.0) is blocked by this issue.

@alexander-akait
Copy link
Member

@bbortt release will be in next couple hours

@bbortt
Copy link

bbortt commented Jul 26, 2019

Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants