search

shama / webpack-stream

:tropical_drink: Run webpack through a stream interface
MIT License
1.39k stars 122 forks source link

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

Closed thereverand closed 4 years ago

thereverand commented 5 years ago

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 commented 5 years ago

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

shama commented 5 years ago

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 commented 5 years ago

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 commented 4 years ago

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

nfplee commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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