Wait for close event when writing bundles

[mischnic]

↪️ Pull Request

I was getting "no such file or directory " when the bundle report was trying to read an bundle from the disk because the dist dir only contained 'index.js.68061.0.6'.

NodeFS#createWriteStream does the atomic renaming on close, but the PackagerRunner only waits for finish.

parcel/packages/core/fs/src/NodeFS.js

Line 47 in 619cbaf

stream.on('close', () => fs.renameSync(tmpFilePath, filePath));

finish is fired before close

Test instructions

Run rm -rf dist/*; parcel build index.js --no-cache --detailed-report multiple times. Before, that would occasionally result in:

✨ Built in 5.14s
[Error: ENOENT: no such file or directory, open '/Users/niklas/Desktop/will2/dist/index.js'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: '/Users/niklas/Desktop/will2/dist/index.js'
}

[height]

This pull request has been linked to 1 task:

• T-473 Fix target dist dir

Link additional Height tasks by mentioning their task IDs in the pull request title or description, commit messages, or comments.

[mischnic]

Don't know why all tests timeout, it works locally

[parcel-benchmark]

Benchmark Results

Kitchen Sink ✅

Timings

Description	Time	Difference
Cold	6.63s	+49.00ms
Cached	2.36s	-87.00ms

Cold Bundles

Bundle	Size	Difference	Time	Difference
dist/legacy/parcel.d5807e82.webp	102.94kb	+0.00b	228.00ms	-12.00ms 🚀
dist/modern/parcel.d5807e82.webp	102.94kb	+0.00b	227.00ms	-13.00ms 🚀
dist/legacy/index.html	709.00b	+0.00b	678.00ms	+49.00ms ⚠️
dist/modern/index.html	709.00b	+0.00b	675.00ms	+48.00ms ⚠️
dist/modern/styles.cda8c91b.css	78.00b	+0.00b	1.33s	-82.00ms 🚀

Cached Bundles

Bundle	Size	Difference	Time	Difference
dist/legacy/parcel.d5807e82.webp	102.94kb	+0.00b	210.00ms	-68.00ms 🚀
dist/modern/parcel.d5807e82.webp	102.94kb	+0.00b	210.00ms	-69.00ms 🚀
dist/legacy/kitchen-sink.679578a5.js	1.16kb	+0.00b	419.00ms	-168.00ms 🚀
dist/modern/kitchen-sink.90de237b.js	1.16kb	+0.00b	419.00ms	-167.00ms 🚀
dist/legacy/index.html	709.00b	+0.00b	420.00ms	-169.00ms 🚀
dist/modern/index.html	709.00b	+0.00b	418.00ms	-168.00ms 🚀
dist/legacy/styles.afb8e31a.css	78.00b	+0.00b	419.00ms	-168.00ms 🚀
dist/modern/styles.cda8c91b.css	78.00b	+0.00b	418.00ms	-168.00ms 🚀

React HackerNews ✅

Timings

Description	Time	Difference
Cold	9.74s	-612.00ms 🚀
Cached	2.37s	+53.00ms

Cold Bundles

Bundle	Size	Difference	Time	Difference
dist/main/index.js	66.80kb	+0.00b	2.27s	-211.00ms 🚀
dist/module/index.js	37.47kb	+0.00b	2.26s	-218.00ms 🚀

Cached Bundles

No bundle changes detected.

AtlasKit Editor 🚨

Timings

Description	Time	Difference
Cold	FAILED	-0.00ms
Cached	FAILED	-0.00ms

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

Three.js x4 🚨

Timings

Description	Time	Difference
Cold	FAILED	-0.00ms
Cached	FAILED	-0.00ms

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

Click here to view a detailed benchmark overview.

[mischnic]

It seemed like initialStream never fired close on CI, now this essentially does Promise.all([pipedFinished, fsClosed])
🤷‍♂️

[mischnic]

Can someone reproduce this? I have a feeling that this only happened on Node 13 and works with 14 and 12 (so without this PR)

[DeMoorJasper]

@mischnic I've been able to reproduce this very reliably on my Mac using Node 12.9. It did not emit the close event at all, this appeared to be due to it never getting destroyed, which I fixes by enabling autoDestroy.

According to the NodeJS docs this is the default option though so I'm kind of confused why this was necessary at all. Anyway hope this does not have any side effects, tests seem to pass locally for me now.

[mischnic]

Somewhat related: I just got

$ PARCEL_BUNDLE_ANALYZER=1 parcel build .
ENOENT: no such file or directory, open '../dist/repl.HASH_REF_.......css'

I think we want

diff --git packages/reporters/bundle-analyzer/src/BundleAnalyzerReporter.js packages/reporters/bundle-analyzer/src/BundleAnalyzerReporter.js
index df1d39e7..077f6537 100644
--- packages/reporters/bundle-analyzer/src/BundleAnalyzerReporter.js
+++ packages/reporters/bundle-analyzer/src/BundleAnalyzerReporter.js
@@ -22,7 +22,9 @@ export default new Reporter({
       Array<Bundle>,
     > = new DefaultMap(() => []);
     for (let bundle of event.bundleGraph.getBundles()) {
-      bundlesByTarget.get(bundle.target.name).push(bundle);
+      if (!bundle.isInline) {
+        bundlesByTarget.get(bundle.target.name).push(bundle);
+      }
     }
 
     let reportsDir = path.join(options.projectRoot, 'parcel-bundle-reports');

as well?

[DeMoorJasper]

@mischnic that's definitely supposed to exclude inline bundles, like the cli bundlereport does https://github.com/parcel-bundler/parcel/blob/v2/packages/reporters/cli/src/bundleReport.js#L27