Skip to content

Commit 2117bf3

Browse files
motiz88facebook-github-bot
authored andcommitted
Fix edge case causing modules to be incorrectly omitted from delta updates
Summary: 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 test case labelled `'all in one delta, adding the live edge after the dead edge'` would fail without the fixes in this diff. 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. 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')`. 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). ## The fix 1. The assumption that a module that is both "deleted" and "added" in the same update can be dropped from the delta is only correct if the module existed in the graph before the update. If it didn't, we must still mark it as "added". To that end, `traverseDependencies` now expects the *unfiltered* set of paths that changed on disk, so it can determine which modules are new to the graph. (Note that we still only traverse them lazily, if they are reachable from a modified module in the current graph, so perf should not be affected.) 2. There was a missing cleanup of `delta.inverseDependencies` when a module was marked as deleted, so it could not be properly re-added. WARNING: This comes from a lot of debugging, but I am still mostly unfamiliar with this part of the bundler. I'm pretty sure about the *tests* but the fix itself smells like a hack, because we are sort of patching the result right at the end. (And why does it fix the order-sensitivity?...) I would love another pair of eyes to thoroughly review this. Differential Revision: D36371424 fbshipit-source-id: bd43590a50e65e3103659ba413c62ff0fed8e500
1 parent 8a8da7b commit 2117bf3

File tree

4 files changed

+468
-8
lines changed

4 files changed

+468
-8
lines changed

packages/metro/src/DeltaBundler/DeltaCalculator.js

+2-1
Original file line numberDiff line numberDiff line change
@@ -243,7 +243,8 @@ class DeltaCalculator<T> extends EventEmitter {
243243
}
244244

245245
const {added, modified, deleted} = await traverseDependencies(
246-
modifiedDependencies,
246+
// Pass the full set of modified files because they might come into the graph during traversal.
247+
modifiedFiles,
247248
this._graph,
248249
this._options,
249250
);

packages/metro/src/DeltaBundler/__tests__/DeltaCalculator-test.js

+33-3
Original file line numberDiff line numberDiff line change
@@ -350,7 +350,7 @@ describe('DeltaCalculator', () => {
350350
});
351351

352352
expect(traverseDependencies).toHaveBeenCalledTimes(1);
353-
expect(traverseDependencies.mock.calls[0][0]).toEqual(['/bundle']);
353+
expect(traverseDependencies.mock.calls[0][0]).toEqual(new Set(['/bundle']));
354354
});
355355

356356
it('does not traverse a file after deleting it and one of its dependencies', async () => {
@@ -379,7 +379,7 @@ describe('DeltaCalculator', () => {
379379
// Only the /bundle module should have been traversed (since it's an
380380
// inverse dependency of /foo).
381381
expect(traverseDependencies).toHaveBeenCalledTimes(1);
382-
expect(traverseDependencies.mock.calls[0][0]).toEqual(['/bundle']);
382+
expect(traverseDependencies.mock.calls[0][0]).toEqual(new Set(['/bundle']));
383383
});
384384

385385
it('should not do unnecessary work when adding a file after deleting it', async () => {
@@ -404,7 +404,7 @@ describe('DeltaCalculator', () => {
404404
await deltaCalculator.getDelta({reset: false});
405405

406406
expect(traverseDependencies).toHaveBeenCalledTimes(1);
407-
expect(traverseDependencies.mock.calls[0][0]).toEqual(['/foo']);
407+
expect(traverseDependencies.mock.calls[0][0]).toEqual(new Set(['/foo']));
408408
});
409409

410410
it('should not mutate an existing graph when calling end()', async () => {
@@ -417,4 +417,34 @@ describe('DeltaCalculator', () => {
417417

418418
expect(graph.dependencies.size).toEqual(numDependencies);
419419
});
420+
421+
it('should pass all updated paths into traverseDependencies', async () => {
422+
await deltaCalculator.getDelta({reset: false});
423+
424+
fileWatcher.emit('change', {
425+
eventsQueue: [{filePath: '/foo'}, {filePath: '/not-in-bundle'}],
426+
});
427+
428+
traverseDependencies.mockReturnValue(
429+
Promise.resolve({
430+
added: new Map(),
431+
modified: new Map([['/foo', fooModule]]),
432+
deleted: new Set(),
433+
}),
434+
);
435+
436+
const result = await deltaCalculator.getDelta({reset: false});
437+
438+
expect(result).toEqual({
439+
added: new Map(),
440+
modified: new Map([['/foo', fooModule]]),
441+
deleted: new Set(),
442+
reset: false,
443+
});
444+
445+
expect(traverseDependencies.mock.calls.length).toBe(1);
446+
expect(traverseDependencies.mock.calls[0][0]).toEqual(
447+
new Set(['/foo', '/not-in-bundle']),
448+
);
449+
});
420450
});

0 commit comments

Comments
 (0)