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

Fix issue 201 by moving cache to the function. #207

Merged
merged 2 commits into from
Aug 27, 2020

Conversation

thereverand
Copy link
Contributor

Fixing issue 201 by preventing cache.compiler from ever being the same instance for separately created instances of webpack-stream. This issue prevents webpack-stream from compiling the same file for multiple targets.

@thereverand
Copy link
Contributor Author

Test 'subsequent runs served from cache' will never pass with this fix.

@shama
Copy link
Owner

shama commented Oct 13, 2018

We'll need to look into the reasoning behind that test on whether we can change or remove it. Otherwise this change seems to introduce a regression.

@thereverand
Copy link
Contributor Author

What it looks like to me is there was some desire to cache the webpack module and the compiler produced from it across any number of executions. But the solution to that doesn't seem to work the way one would expect. On lines 33,34,35 there is a check if the cache already contains webpack or the same options passed. If not it, it will reinitialize the cache ( = {} ). Then on lines 48.49, it caches the compiler if it hasn't already been cached, and then re-caches it. What it looks like is happening is the cached compiler remains even if the cache object is emptied when I compile multiple files using Gulp 4, even is the tasks are placed in series. A part of the problem is Gulp 4 series tasks have some overlap, but with different options: the compiler shouldn't be cached for their to be multiple calls to the same instance.

@Miladiir
Copy link

Any updates on this PR? We are affected by #201 and would like to update. Currently stuck on 5.0.0.

@nfplee
Copy link

nfplee commented Aug 27, 2020

I'm interested in an update aswell. This makes working with multiple bundles impossible and this fix solves the problem for me. Any idea when it will be merged?

@thereverand do you know a solution which doesn't require forking this library? I tried adding a timeout between my calls but this hasn't made a difference.

@shama
Copy link
Owner

shama commented Aug 27, 2020

Since nobody responded about this possibly introducing a regression for them on #109 I'll go ahead and merge this today. FWIW, I still don't understand the use case for #109 or this.

@nfplee
Copy link

nfplee commented Aug 27, 2020

@shama. Great thanks. The possible regression appears to be related to caching and is less important than this bug.

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.

None yet

4 participants