Skip to content

Commit 6d68720

Browse files
motiz88facebook-github-bot
authored andcommitted
Fix batching bugs causing modules to be incorrectly omitted from delta updates (#819)
Summary: Pull Request resolved: #819 Fixes a bug in `DeltaCalculator` / `traverseDependencies` that was causing modules to be incorrectly omitted from the bundle while other modules were still depending on them. ## The bug Specifically, the following new test cases would fail without the fixes in this diff: ### Test: "all in one delta, adding the live edge after the dead edge" In the test, we perform the following sequence of mutations on an initial graph, and then compute the delta between the initial and final states of the graph: 1. Create the file `/quux`. 2. Add a dependency between `/foo` and `/quux`, marking `/foo` as modified. 3. Add a dependency between `/bundle` and `/quux`, marking `/bundle` as modified. 4. Remove the pre-existing dependency between `/bundle` and `/foo`. {F731851675} With `/bundle` as the entry point, this should leave us with a graph consisting of `/bundle` and `/quux`. Before this diff, `traverseDependencies` would omit `/quux` because it incorrectly concluded that `/quux` must have been present in the initial graph. NOTE: In the test code, the terms "live edge" and "dead edge" refer to the fact that the edge between `/bundle` and `/quux` is "live" i.e. present in the final graph, while the edge between `/foo` and `/quux` is "dead" i.e. removed from the final graph (because `/foo` itself is pruned). ### Test: "more than two state transitions in one delta calculation" Perform the following sequence of operations (from the same initial graph as the other test): 1. Create the file `/quux`. 2. **Process the filesystem change event**, i.e. compute an empty delta (since `/quux` is not reachable). 3. Add a dependency between `/bar` and `/quux`, marking `/bar` as modified. 4. Remove the pre-existing dependency between `/foo` and `/bar`, marking `/foo` as modified. 5. Add a dependency between `/bundle` and `/quux`, marking `/bundle` as modified. 6. Compute another delta representing the mutations since step 2. {F732573059} As with the other test, before the fix, `/quux` would be incorrectly omitted from the final delta. ### Impact The bug would typically manifest as various runtime errors after a large filesystem update (e.g. rebasing) due to modules being missing from the module registry on the client. In our example, we would not send down the code for `/quux`, so we can expect evaluating `/bundle` to fail when it tries to invoke the equivalent of `require('/quux')`. ## The fix The root of the problem is that we record deletions and additions on the same module during traversal, and then try to disambiguate them in a postprocessing pass using incomplete information. Instead, we now track the correct delta state throughout the algorithm, with addition and deletion being mutually exclusive. Note that an earlier version of this diff contained the wrong solution: > `traverseDependencies` now expects the *unfiltered* set of paths that changed on disk, **so it can determine which modules are new to the graph**. The second test above shows why this would be an incorrect heuristic: The filesystem change affecting `/quux` can occur in a separate `traverseDependencies` run, so we can't use that to disambiguate dependencies that appear to be both deleted+added. The right fix is to track the state correctly to begin with. Reviewed By: robhogan Differential Revision: D36371424 fbshipit-source-id: a64b9bec45ba2a6558e671a2bd686fc80e24cada
1 parent b2e86cd commit 6d68720

File tree

2 files changed

+543
-31
lines changed

2 files changed

+543
-31
lines changed

0 commit comments

Comments
 (0)