shellscape / webpack-manifest-plugin

webpack plugin for generating asset manifests
MIT License
1.44k stars 184 forks source link

fix: make sure manifest writes don't overlap #56

Closed mastilver closed 7 years ago

mastilver commented 7 years ago

closes #21

codecov-io commented 7 years ago

Codecov Report

Merging #56 into master will decrease coverage by 0.93%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #56      +/-   ##
==========================================
- Coverage     100%   99.06%   -0.94%     
==========================================
  Files           2        2              
  Lines          56      107      +51     
==========================================
+ Hits           56      106      +50     
- Misses          0        1       +1
Impacted Files Coverage Δ
lib/plugin.js 99.05% <100%> (-0.95%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c441207...29ffc01. Read the comment docs.

zuzusik commented 7 years ago

The main question here - does it really fix #21? I don't see an appropriate test case added here. If it is hard to reproduce maybe we should ask reporters of #21 to test this fix with their local setup?

If it doesn't fix #21 then I don't see why it should be merged. As a step for #33 as supposed here https://github.com/danethurber/webpack-manifest-plugin/issues/33#issuecomment-314063783 ? Don't think we need this step - we better fix #33 with one monolithic PR

mastilver commented 7 years ago

@zuzusik Thank you for reviewing this PR :)

Did you see #49, I think you could be a good help for me

I think I know what's causing the issue, I will try to write a test that reproduce it

zuzusik commented 7 years ago

Also added few style related comments here

zuzusik commented 7 years ago

One serious concern here: https://github.com/danethurber/webpack-manifest-plugin/pull/56/files#r126405642

mastilver commented 7 years ago

@zuzusik PR updated, do you mind having another look

mastilver commented 7 years ago

@zuzusik I'm not relying anymore or memory-fslogic

Let me know what you think!