cascornelissen / svg-spritemap-webpack-plugin

SVG spritemap plugin for webpack
MIT License
210 stars 51 forks source link

WebpackManifestPlugin output differs between initial compile and subsequent recompiles in watch mode #157

Closed pjho closed 3 years ago

pjho commented 3 years ago

Description On build and initial compile in watch mode Webpack spritemap is written to manifest as:

{"<settings.filename>": "/path/to/spritemap.svg"}.

On subsequent rebuilds it is written as:

{"<settings.chunk.name>": "/path/to/spritemap.svg"}

Expected behavior I would expect both outputs to be the same. Ideally recorded as <settings.chunk.name>.svg

Actual behavior see description above

System information

  System:
    OS: macOS 10.15.6
    CPU: (4) x64 Intel(R) Core(TM) i5-7360U CPU @ 2.30GHz
    Memory: 2.15 GB / 16.00 GB
    Shell: 5.7.1 - /bin/zsh
  Binaries:
    Node: 12.18.4 - ~/.nvm/versions/node/v12.18.4/bin/node
    Yarn: 1.22.4 - /usr/local/bin/yarn
    npm: 6.14.6 - ~/.nvm/versions/node/v12.18.4/bin/npm
  npmPackages:
    @babel/core: ^7.13.10 => 7.13.10
    svg-spritemap-webpack-plugin: ^3.9.0 => 3.9.0
    webpack: ^5.24.4 => 5.24.4
    webpack-cli: ^4.5.0 => 4.5.0
    webpack-manifest-plugin: ^3.1.0 => 3.1.0

Minimal reproduction https://github.com/pjho/svg-spritemap-webpack-plugin-example

Additional context Steps to reproduce: Using repo above

npm run build

manifest.json naming = {"build/svgsp-filename.svg": "/build/svgsp-filename.svg"}

npm run watch

manifest.json naming = {"build/svgsp-filename.svg": "/build/svgsp-filename.svg"}

Modify src/js/site.js to trigger rebuild

manifest.json naming = {"svgsp-chunkname.svg": "/build/svgsp-filename.svg"}

Personally I find using the chunkname in the manifest is preferable as it means output path is left off and I can simply reference manifest['chunkname.svg'] to get the output path of my svg sprite including paths and hashes.

cascornelissen commented 3 years ago

I'm not entirely sure how webpack-manifest-plugin works internally but is this difference also visible in the Webpack output in your terminal or not? This plugin "just" adds a file to the internal Webpack compilation object and I'd guess the webpack-manifest-plugin uses that same object so I'm not entirely sure where this is going wrong 🤔

cascornelissen commented 3 years ago

I just noticed you included a repository and reproduction steps, thanks for that!

I think where this is going "wrong" is the fact that you're using paths in both the output.filename Webpack option and the output.filename SVG Spritemap Webpack Plugin configuration. I wasn't expecting that this would even work (and I still think it's strange, why include a path in something called "filename") but the Webpack docs at least include:

Note this option is called filename but you are still allowed to use something like 'js/[name]/bundle.js' to create a folder structure.

The plugin doesn't "allow" this as-is (apparently) but I'm open to a PR that includes the required changes to make this work as one would expect which is indeed having the filename without the path (chunk name) as the key.

pjho commented 3 years ago

Thanks for taking a look. I'll see if I can tidy up my webpack settings. I agree it is odd specifying path in the filename option - the purpose being to keep output assets in tidy folders but a) there may be better way to configure that and b) it probably doesn't actually matter since its compiled output and path is referenced by manifest file. Anyways, failing that I'll try and take a look at a pr. About time I dig a bit deeper into webpack plugin internals :D

Feel free to close this issue and I'll take a look when I next get a moment 👍

pjho commented 3 years ago

hmm. Even commenting out both filename settings in webpack config (webpack and SVGSpritemap settings) I get the same behavior. And there is a difference in the output in webpack terminal. On the recompile spritemap.svg is missing from the output files - or is it that top line which is unamed with the [cached] flag.

Screen Shot 2021-03-17 at 9 28 09 AM

On recompile in the output manifest file the svg sprite is referenced as {"svgsp-chunkname.svg": "/spritemap.svg"} whereas on a single/initial build it is referenced as {"spritemap.svg": "/spritemap.svg"}

Anyways just reporting findings, will continue to dig. Cheers again for your work on this plugin it makes for a really nice svg icon workflow 👍

cascornelissen commented 3 years ago

Hmm, that's really interesting 🤔 I was sure I checked it without the path as well, must've overlooked something. Will take a better look at it when I find the time, thanks for spending some time on the debugging on your side! ❤️

cascornelissen commented 3 years ago

I've figured out where this is going wrong, it's a combination of two things.

1. An asset with a chunk in svg-spritemap-webpack-plugin This plugin creates an asset (the spritemap SVG) and adds a chunk to it to ensure the asset itself is not getting cleaned up by Webpack because it's not attached to anything. This is done in the beforeChunks hook which IIRC is the earliest possible moment to do this...

https://github.com/cascornelissen/svg-spritemap-webpack-plugin/blob/bcc7f5139849913d7282c27342e8ee9e482026a3/lib/index.js#L219-L235

2. Entry asset detection in webpack-manifest-plugin When debugging webpack-manifest-plugin, the initial build when running npm run watch contains an asset without the chunk, while all subsequent runs do contain the chunk, causing this statement to toggle from false on the initial run to true on subsequent runs which in turn explains why it toggles the key of in the generated manifest.

Solutions Now here's the hard part, I'm not sure which of these parts is "wrong" or what should be fixed but I see three possibilities.

  1. The isEntryAsset in webpack-manifest-plugin is improved. I don't think an asset that does have chunks is an entry asset by definition but I'm not sure of that and going through the history of that function doesn't provide much additional value either.
  2. We somehow fix the fact that the asset does not yet contain the chunk in the first compilation. I'm not sure how to since AFAIK beforeChunks is the earliest hook where this works.
  3. As a short-term workaround you could set both output.filename and output.chunk.name to the same value? Unless you're using hashes in the filename, of course.

Since I believe option 1 is the most straightforward solution, an issue could be created on the webpack-manifest-plugin repository. Otherwise, feel free to try and figure out how the asset with chunk can be generated early enough that webpack-manifest-plugin works correctly.