Fix postProcessors race condition

[hershmire]

The Problem

When adding a CSS post processor to Cartero's postProcessors option, I get a race condition where Cartero writes to the assetsRequiredByEntryPoint object of the metaData.json file prior to the CSS post process stream finishing. Since Cartero only checks that all the entry point JS bundles and parcelify done event is called, there is sometimes a chance for a post processed stream to finish too late, resulting in empty arrays for required assets.

To easily simulate this, I've added a dummy postProcessor stream with a setTimeout() (part of test case) to delay the ending of the stream.

metaData.json Before Fix

{
  "formatVersion": 4,
  "packageMap": {
    "node_modules/handlebars": "a3b2f7a6be3be8bc3f66336872e8d93019741ac9",
    "node_modules/hbsfy": "2f526e43093050421dcd4eb3beb31d4c7420c23d",
    "page1": "cc65288ab19af1c5bc3fc1efbcf4a9ac57f144e5",
    "page2": "e32005f31243d05a47f3d10cd40afa85161fcbf9"
  },
  "entryPointMap": {
    "page1/page1.js": "cc65288ab19af1c5bc3fc1efbcf4a9ac57f144e5",
    "page2/page2.js": "e32005f31243d05a47f3d10cd40afa85161fcbf9"
  },
  "assetsRequiredByEntryPoint": {
    "page2/page2.js": {
      "script": [
        "e32005f31243d05a47f3d10cd40afa85161fcbf9/page2_bundle_14f8371b21a34520c5e5d99c81f2ae7502284f8a.js"
      ],
      "style": [],
      "image": []
    },
    "page1/page1.js": {
      "script": [
        "cc65288ab19af1c5bc3fc1efbcf4a9ac57f144e5/page1_bundle_cd4a70b7ddb121e2ce6e7bf282a7fa88f64424f2.js"
      ],
      "style": [],
      "image": [
        "cc65288ab19af1c5bc3fc1efbcf4a9ac57f144e5/page1_da39a3ee5e6b4b0d3255bfef95601890afd80709.gif"
      ]
    }
  },
  "assetMap": {
    "page1/page1.gif": "cc65288ab19af1c5bc3fc1efbcf4a9ac57f144e5/page1_da39a3ee5e6b4b0d3255bfef95601890afd80709.gif"
  }
}

Note: assetsRequiredByEntryPoint.page1/page1.js's style array should not be empty. However, the style and image arrays for the page2/page2.js's object is correct since I don't have any css or images in there.

metaData.json After Fix

{
  "formatVersion": 4,
  "packageMap": {
    "node_modules/handlebars": "a3b2f7a6be3be8bc3f66336872e8d93019741ac9",
    "node_modules/hbsfy": "2f526e43093050421dcd4eb3beb31d4c7420c23d",
    "page1": "cc65288ab19af1c5bc3fc1efbcf4a9ac57f144e5",
    "page2": "e32005f31243d05a47f3d10cd40afa85161fcbf9"
  },
  "entryPointMap": {
    "page1/page1.js": "cc65288ab19af1c5bc3fc1efbcf4a9ac57f144e5",
    "page2/page2.js": "e32005f31243d05a47f3d10cd40afa85161fcbf9"
  },
  "assetsRequiredByEntryPoint": {
    "page2/page2.js": {
      "script": [
        "e32005f31243d05a47f3d10cd40afa85161fcbf9/page2_bundle_14f8371b21a34520c5e5d99c81f2ae7502284f8a.js"
      ],
      "style": [],
      "image": []
    },
    "page1/page1.js": {
      "script": [
        "cc65288ab19af1c5bc3fc1efbcf4a9ac57f144e5/page1_bundle_cd4a70b7ddb121e2ce6e7bf282a7fa88f64424f2.js"
      ],
      "style": [
        "cc65288ab19af1c5bc3fc1efbcf4a9ac57f144e5/page1_bundle_58c4527bcf7f0a439145e906103df9afa2197119.css"
      ],
      "image": [
        "cc65288ab19af1c5bc3fc1efbcf4a9ac57f144e5/page1_da39a3ee5e6b4b0d3255bfef95601890afd80709.gif"
      ]
    }
  },
  "assetMap": {
    "page1/page1.gif": "cc65288ab19af1c5bc3fc1efbcf4a9ac57f144e5/page1_da39a3ee5e6b4b0d3255bfef95601890afd80709.gif"
  }
}

Note: The style array is now correct for page1/page1.js:

"style": [ 
  "cc65288ab19af1c5bc3fc1efbcf4a9ac57f144e5/page1_bundle_58c4527bcf7f0a439145e906103df9afa2197119.css" 
]

Solution

Add an additional check to make sure any postProcessor parcels are finished before writing to metaData.json.

Changes

• Check for postProcessor bundles to be finished before writing assetsRequiredByEntryPoint to metaData.json
• Test case for postProcessor race condition
• Code consolidation for async parallel tasks

[ferlores]

hey @dgbeck, can you please check this PR? This problem is kind of blocking us. Thanks!

[dgbeck]

Hi @hershmire and @ferlores thanks for the thorough write up and the PR. I looked at this a bit today and it looks good. I'd just like to take some more time to review it before merging and also would like to test it using a large project to make sure nothing breaks. I will get to this by early next week if not sooner.

Thanks!

[hershmire]

Thanks @dgbeck! Any progress on taking a closer look at the PR and testing on a larger project?

[dgbeck]

Hi @hershmire , yes we are making progress. Looking good, will update you later today.

[dgbeck]

Hi @hershmire this is a great find. Thank you very much for reporting this issue and for the very clear explanation and also the new test, which made it very easy to reproduce.

As we started to review this PR we took a step back and see if there is a way to simplify the way we were handling these async post processing operations. Also, we have been considering adding support for a css common bundle, as suggested in #48, and did some refactoring with that eventual goal in mind.

The result is this PR. We took the async stuff out of the bundleWritten handler completely (except for in watch mode), waiting until parcelify has emitted its done event before writing the meta data file. Also we have expanded the common bundle support to a tempCommonBundles and this.finalCommonBundles hash that can be used to track common bundles for additional asset types. Tests, including the new one in this PR, are passing, and we have put the new logic through its paces (including watch mode).

Can you check out #55 and confirm it is working for your use case as well? I can then merge to master and do a patch release.

Thank you very much for identifying and helping resolve this issue!

[hershmire]

@dgbeck Awesome, everything seems to work as expected on our end! I think we're a go.

When could we have a patch published to NPM?

[dgbeck]

published @6.2.1. Thanks again @hershmire !

[hershmire]

👍