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

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

gluck
Copy link
Contributor

@gluck gluck commented Mar 10, 2020

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.

Fixes GoogleChromeLabs#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.
@developit
Copy link
Collaborator

developit commented Mar 10, 2020

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.

@developit developit self-requested a review March 10, 2020 22:04
developit and others added 4 commits March 10, 2020 18:18
- chunkFilename is preserved from compilerOptions
- use chunkName = worker name (or ID) + '.worker'
- allow to override chunkFilename is needed (GoogleChromeLabs#19)
@gluck gluck changed the title Change worker chunk name from <ID> to w<ID> Change worker chunk name from <ID> to <ID>.worker Mar 11, 2020
@gluck
Copy link
Contributor Author

gluck commented Mar 11, 2020

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
Copy link
Contributor Author

gluck commented Mar 11, 2020

(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
Copy link
Collaborator

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.

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.

Error when using dynamic imports
2 participants