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

mastilver · 2017-07-09T14:40:24Z

closes #21

codecov-io · 2017-07-09T14:50:58Z

Codecov Report

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

@@            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%)`	⬇️

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 · 2017-07-10T10:43:19Z

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
#33 (comment) ?
Don't think we need this step - we better fix #33 with one monolithic PR

zuzusik · 2017-07-10T10:43:40Z

lib/plugin.js


+    const shouldUseNodeFs = compiler.outputFileSystem.constructor.name !== 'MemoryFileSystem' ||
+                                      this.opts.writeToFileEmit;
+    if (shouldUseNodeFs) {


As far as I see, the logic used here breaks what was achieved with #28, as it assumes the file should be output to both memory and file system - now when writeToFileEmit is set to true, it will be output only to file system, and not to memory.

👍 will fix that

mastilver · 2017-07-10T11:12:28Z

@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 · 2017-07-10T11:53:53Z

lib/plugin.js


+    const shouldUseNodeFs = compiler.outputFileSystem.constructor.name !== 'MemoryFileSystem' ||
+                                      this.opts.writeToFileEmit;


this indentation looks broken, I think we should stick to 2 spaces everywhere, also I think it is little bit more readable if we put || on the new line:

const shouldUseNodeFs = compiler.outputFileSystem.constructor.name !== 'MemoryFileSystem' || this.opts.writeToFileEmit;

zuzusik · 2017-07-10T11:57:03Z

spec/plugin.integration.spec.js

+
+var ManifestPlugin = require('../index.js');
+
+function webpackConfig (webpackOpts, opts) {


let's be consistent with style here - we should stick to function<name>(...) {} style as it is used all over the project

I mean - there shouldn't be a space before (

Also it should be fixed in other places in this PR

zuzusik · 2017-07-10T11:58:26Z

lib/plugin.js


+    const shouldUseNodeFs = compiler.outputFileSystem.constructor.name !== 'MemoryFileSystem' ||


I know that const is much more appropriate here, but this file uses var all over the place (and even 2 previous lines use var)

So I guess we should stick to var and if we want to go with const we should create separate PR in which we do refactor all var to const

Also, as far as I see we do rely on internal implementation of memory-fs here. Not sure if it is a good idea - the class name can be changed at any time without implicating the API, so it wouldn't be a reason for new major version release.

And another question here - what if someone provides custom memory file system which is implemented in the other way?

I agree it's ugly but that the only way I could find to check if it's really MemoryFileSystem, if you have a better solution feel free to share it ;)

I'm note the only one doing it: https://github.com/gajus/write-file-webpack-plugin/blob/51167a8cf33ee0885805abd1fb85c8306f0eda27/src/index.js#L19

What do you think about this proposal:

var outputFolder = compilation.options.output.path; var outputFile = path.join(outputFolder, this.opts.fileName); compiler.outputFileSystem.mkdirp.sync(path.dirname(outputFile)); compiler.outputFileSystem.writeFileSync(outputFile, json); if (this.opts.writeToFileEmit) { fse.outputFileSync(outputFile, json); }

That would only work when using memory-fs, see: https://github.com/webpack/webpack/blob/master/lib/node/NodeOutputFileSystem.js#L17
It doesn't have writeFileSync

this should work thought:

compiler.outputFileSystem.mkdirp.sync(path.dirname(outputFile)); if (isMemoryFs) { compiler.outputFileSystem.writeFileSync(outputFile, json); } if (!isMemoryFs || this.opts.writeToFileEmit) { fse.outputFileSync(outputFile, json); }

Your code looks good, should work as expected 👍 I only assume it should use mkdirp.sync

What I want to achieve here is that we can skip compiler.outputFileSystem.constructor.name !== 'MemoryFileSystem' check

Overall impression - it is weird that different FSs used in WebPack have different sets of methods.

I only assume it should use mkdirp.sync

Damn, I updated it :)

zuzusik · 2017-07-10T11:59:06Z

spec/plugin.spec.js

@@ -62,7 +62,7 @@ function webpackCompile(webpackOpts, opts, cb) {
  var fs = compiler.outputFileSystem = new MemoryFileSystem();

  compiler.run(function(err, stats){
-    var manifestFile = JSON.parse( fs.readFileSync(manifestPath).toString() );
+    var manifestFile = JSON.parse(fs.readFileSync(manifestPath).toString());


I'd remove this changes as they are completely unrelated to this PR

zuzusik · 2017-07-10T11:59:32Z

Also added few style related comments here

zuzusik · 2017-07-10T12:27:34Z

lib/plugin.js

      fse.outputFileSync(outputFile, json);
+    } else {
+      compiler.outputFileSystem.mkdirpSync(path.dirname(outputFile));


I wonder why mkdirpSync and not mkdirp?
Webpack docs only mention mkdirp here so it might blow up for some custom FS that provides only mkdirp but doesn't provide mkdirpSync

Should be mkdirp.sync

zuzusik · 2017-07-10T12:28:34Z

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

mastilver · 2017-07-12T09:40:20Z

@zuzusik PR updated, do you mind having another look

mastilver · 2017-07-16T12:12:37Z

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

Let me know what you think!

mastilver force-pushed the write-file-from-plugin branch 2 times, most recently from ce697a8 to 36d0f9b Compare July 9, 2017 20:23

This was referenced Jul 9, 2017

Improve tests coverage #51

Closed

an option to create the manifest json to specified directory #33

Closed

zuzusik reviewed Jul 10, 2017

View reviewed changes

mastilver force-pushed the write-file-from-plugin branch 2 times, most recently from d334049 to 81bdf52 Compare July 12, 2017 09:39

mastilver force-pushed the write-file-from-plugin branch 2 times, most recently from 4f0347c to 3f9f027 Compare July 16, 2017 12:11

mastilver force-pushed the write-file-from-plugin branch 2 times, most recently from 0059180 to 609b6ca Compare July 16, 2017 12:18

mastilver changed the title ~~fix: write file directly from the plugin~~ fix: make sure manifest writes don't overlap Jul 16, 2017

mastilver force-pushed the write-file-from-plugin branch from 609b6ca to b8912b5 Compare July 16, 2017 12:20

fix: make sure manifest writes don't overlap

cb266bc

mastilver force-pushed the write-file-from-plugin branch from b8912b5 to cb266bc Compare July 16, 2017 12:22

mastilver requested a review from zuzusik July 20, 2017 15:55

mastilver requested review from gmac and a-x- July 28, 2017 17:04

mastilver added this to the v1.3 milestone Jul 29, 2017

mastilver added 2 commits August 9, 2017 21:43

Merge branch 'master' into write-file-from-plugin

1936563

fix: make sure absolute file path is respected

29ffc01

mastilver merged commit d6a20cf into master Aug 9, 2017

mastilver deleted the write-file-from-plugin branch August 9, 2017 20:49

mastilver mentioned this pull request Aug 14, 2017

manifest is not added to compilation.assets anymore #72

Closed

snyk-bot mentioned this pull request Feb 24, 2020

[Snyk] Upgrade webpack-manifest-plugin from 1.2.1 to 1.3.2 DavidKindler/burger-builder#31

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mastilver commented Jul 9, 2017

codecov-io commented Jul 9, 2017 •

edited

zuzusik commented Jul 10, 2017

zuzusik Jul 10, 2017 •

edited

mastilver Jul 10, 2017

mastilver commented Jul 10, 2017

zuzusik Jul 10, 2017

zuzusik Jul 10, 2017

zuzusik Jul 10, 2017

zuzusik Jul 10, 2017

zuzusik Jul 10, 2017

mastilver Jul 10, 2017

zuzusik Jul 10, 2017 •

edited

mastilver Jul 10, 2017

mastilver Jul 10, 2017 •

edited

zuzusik Jul 10, 2017

mastilver Jul 10, 2017

zuzusik Jul 10, 2017

zuzusik commented Jul 10, 2017

zuzusik Jul 10, 2017

mastilver Jul 10, 2017

zuzusik commented Jul 10, 2017

mastilver commented Jul 12, 2017

mastilver commented Jul 16, 2017


		const shouldUseNodeFs = compiler.outputFileSystem.constructor.name !== 'MemoryFileSystem' \|\|
		this.opts.writeToFileEmit;


		var ManifestPlugin = require('../index.js');

		function webpackConfig (webpackOpts, opts) {

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

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

Conversation

mastilver commented Jul 9, 2017

codecov-io commented Jul 9, 2017 • edited

Codecov Report

zuzusik commented Jul 10, 2017

zuzusik Jul 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mastilver commented Jul 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuzusik Jul 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mastilver Jul 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuzusik commented Jul 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuzusik commented Jul 10, 2017

mastilver commented Jul 12, 2017

mastilver commented Jul 16, 2017

codecov-io commented Jul 9, 2017 •

edited

zuzusik Jul 10, 2017 •

edited

zuzusik Jul 10, 2017 •

edited

mastilver Jul 10, 2017 •

edited