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

Save the exported method names in a local cache object #61

Merged
merged 1 commit into from
Feb 21, 2020

Conversation

danieldunderfelt
Copy link
Contributor

@danieldunderfelt danieldunderfelt commented Jan 2, 2019

Save the exported method names in a local cache object, per entry file, instead of using the compilation object. Fixes #24 where worker methods are undefined after a recompile.

Since this is the first webpack loader I've touched, this PR might need further changes. But using the CACHE object like this works, as opposed to using the compilation object which doesn't work after a recompile.

This PR is meant to illustrate the problem, there might be a better way to fix it.

…e, instead of using the compilation object. Fixes developit#24 where worker methods are undefined after a recompile.
@@ -83,7 +83,7 @@ loader.pitch = function(request) {
// only process entry exports
if (current.resource!==entry) return;

let exports = compilation.__workerizeExports || (compilation.__workerizeExports = {});
let exports = CACHE[entry] || (CACHE[entry] = {});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The core issue is here. The __workerizeExports property does not seem to be persisted across re-compiles, resulting in an empty methods list. I propose using the CACHE object instead, since it seems to be used for this purpose anyway.

I assume the compilation object is somehow instanced by entry file, so I wanted to replicate that in the CACHE object if there are many workers in the project.

I think this will still require the end-user to restart the build when method names change, but that's a lot better than having to restart after any change. AFAIK the generated filename isn't available at this point yet, and this function only runs once so it wouldn't know what the next filename is after a recompile. Feel free to suggest modifications to this to enable recompiling method name changes without a restart.

@@ -106,8 +106,9 @@ loader.pitch = function(request) {
if (entries[0]) {
worker.file = entries[0].files[0];

let entry = entries[0].entryModule.resource;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CACHE is keyed by the resource name, so we need to retrieve that here.

Copy link
Owner

@developit developit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Only concern I have is: what about when the exports change?

@developit
Copy link
Owner

Apologies for the long lead time on this PR. I've been pondering how to do this without relying on static exports. It might be some combination of this and inspecting compilation.name.

@bengry
Copy link

bengry commented Oct 31, 2019

@danieldunderfelt @developit is this PR still active? We're having the issue mentioned in #24 and wanted to know if this is going to be solved soon, or if we should fork the package (essentially merging this PR)/move to another one for web workers in Webpack?

@exarus
Copy link

exarus commented Dec 16, 2019

Any updates on this @developit @danieldunderfelt ?

@danieldunderfelt
Copy link
Contributor Author

@exarus I don't know, it is in the hands of @developit. I'm not actively using this package at the moment. I recommend patch-package if you need to hotpatch a dependency.

@bengry
Copy link

bengry commented Dec 17, 2019

Thanks @danieldunderfelt. I created a patch for this, available here if anyone wants to use it until this PR is merged.
Just put it under <rootDir>/patches/workerize-loader+1.1.0.patch, after following the steps in patch-package.

Copy link
Owner

@developit developit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the super long delay!

@developit developit merged commit d7c85f9 into developit:master Feb 21, 2020
@bengry
Copy link

bengry commented Feb 21, 2020

@developit Thanks for merging this. Is a new version of workerize-loader going to be release on npm soon? The latest at the moment is still 1.1.0 (published ~6 months ago).
It'd be great to drop the patch mentioned here that we currently use.

@bengry
Copy link

bengry commented May 7, 2020

@developit Thanks for merging this. Is a new version of workerize-loader going to be release on npm soon? The latest at the moment is still 1.1.0 (published ~6 months ago).
It'd be great to drop the patch mentioned here that we currently use.

@developit Is a v1.1.1 (or otherwise) coming with this? It's been over 2 months since this PR was merged to master, but no new version was released on npm.

@developit
Copy link
Owner

Hi @bengry - sorry about that, I'll get a release out today.

@developit
Copy link
Owner

Published in 1.2.0.

@bengry
Copy link

bengry commented May 8, 2020

Thanks. Seems like it raised another issue for people though - #85.
I haven't upgraded yet, but seems like it happened to a few people already

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.

Empty Worker returned after hot-reloading
5 participants