GoogleChromeLabs / worker-plugin

👩‍🏭 Adds native Web Worker bundling support to Webpack.
https://npm.im/worker-plugin
Apache License 2.0
1.92k stars 79 forks source link

Change worker chunk name from <ID> to <ID>.worker #60

Open gluck opened 4 years ago

gluck commented 4 years ago

Fixes #43, as it avoids conflicts with other number-based webpack chunks. Note that the conflict may not be on the filename (since this plugin adds '.worker' to it) but on the internal key used by webpack in referencing modules.

E.g. in the below generated webpack loader code, using w0 avoids conflict with chunkId 0 that would otherwise never load.

image

I'd argue that a cleaner "fix" would be to name the chunk with something like worker.${id} and drop the chunkFilename replacement logic (that inserts .worker. before the output extension), but that'd break the naming of those who provided a name to the worker, so you tell me.

developit commented 4 years ago

TBH I think I agree with your point about the cleaner fix @gluck - it'll break existing names, but perhaps that's okay? Could even append .worker to the name, then remove .worker.worker as a way to make it backwards-compatible.

I just published 4.0.0 - if we can make a call on this relatively soon, I'd be okay with doing this as a minor 4.1 since nobody has yet upgraded.

gluck commented 4 years ago

Done, I was zealous and included suggested #19 change at the same time (because a user could use this feature to revert/customize the naming behavior), lmk if you think I should remove it.

gluck commented 4 years ago

(note that I couldn't make it backward-compat because the check for duplicate worker.worker. would have to be done after webpack has resolved chunkFilename and I don't know how to hook there). I could inject into .worker into chunkFilename if it doesn't include [name] to make sure output files always include .worker in them if you think it's a good idea, but I'm personally not sure it is.

developit commented 4 years ago

I think this looks good! I'll need a little bit before I can do a proper thorough review. We'll want to think about whether this constitutes a v5 or not given the naming change.