Skip to content

Commit

Permalink
Handle correctly multiple requires to the same file with different forms
Browse files Browse the repository at this point in the history
Summary:
The traverse dependencies logic was using the absolute path of the resolved dependency to group the module dependencies, so for example if there's a module like:

```
const a = require('./a');
const a1 = require('./a.js');
```

The traversal dependencies logic was just outputting a single dependency for that module.

Since we're transforming each `require()` call to replace the relative path by a dependencyMap, the code from above was transformed to:

```
const a = require(_dependencyMap[0]);
const a1 = require(_dependencyMap[1]);
```

But since the traverse dependencies logic could only find a single dependency, `_dependencyMap[1]` was undefined, causing a runtime error.

This fixes #152 (more info in the task)

Reviewed By: jeanlauliac

Differential Revision: D7258093

fbshipit-source-id: 65c42b87e589430ecc96b906230dd7c4c55c2146
  • Loading branch information
rafeca authored and facebook-github-bot committed Mar 14, 2018
1 parent 6a587db commit 233b00e
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 21 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,28 @@ describe('edge cases', () => {
]);
});

it('should create two entries when requiring the same file in different forms', async () => {
const edges = new Map();
await initialTraverseDependencies('/bundle', dependencyGraph, {}, edges);

// We're adding a new reference from bundle to foo.
Actions.addDependency('/bundle', '/foo', 0, 'foo.js');

expect(
getPaths(
await traverseDependencies([...files], dependencyGraph, {}, edges),
),
).toEqual({
added: new Set(['/bundle']),
deleted: new Set(),
});

expect([...edges.get(entryModule.path).dependencies]).toEqual([
['foo.js', '/foo'],
['foo', '/foo'],
]);
});

it('should traverse the dependency tree in a deterministic order', async () => {
// Mocks the shallow dependency call, always resolving the module in
// `slowPath` after the module in `fastPath`.
Expand Down
44 changes: 23 additions & 21 deletions packages/metro/src/DeltaBundler/traverseDependencies.js
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ async function processEdge(
onDependencyAdd: () => mixed,
onDependencyAdded: () => mixed,
): Promise<void> {
const previousDependencies = new Set(edge.dependencies.values());
const previousDependencies = edge.dependencies;

const result = await dependencyGraph
.getModuleForPath(edge.path)
Expand All @@ -222,12 +222,12 @@ async function processEdge(
edge.output.source = result.source;
edge.dependencies = new Map();

currentDependencies.forEach((relativePath, absolutePath) => {
currentDependencies.forEach((absolutePath, relativePath) => {
edge.dependencies.set(relativePath, absolutePath);
});

for (const absolutePath of previousDependencies.values()) {
if (!currentDependencies.has(absolutePath)) {
for (const [relativePath, absolutePath] of previousDependencies) {
if (!currentDependencies.has(relativePath)) {
removeDependency(edge, absolutePath, edges, delta);
}
}
Expand All @@ -236,22 +236,24 @@ async function processEdge(
// added and removed dependency, to get all the modules that have to be added
// and removed from the dependency graph.
await Promise.all(
Array.from(currentDependencies.keys()).map(async absolutePath => {
if (previousDependencies.has(absolutePath)) {
return;
}

await addDependency(
edge,
absolutePath,
dependencyGraph,
transformOptions,
edges,
delta,
onDependencyAdd,
onDependencyAdded,
);
}),
Array.from(currentDependencies.entries()).map(
async ([relativePath, absolutePath]) => {
if (previousDependencies.has(relativePath)) {
return;
}

await addDependency(
edge,
absolutePath,
dependencyGraph,
transformOptions,
edges,
delta,
onDependencyAdd,
onDependencyAdded,
);
},
),
);
}

Expand Down Expand Up @@ -370,12 +372,12 @@ function resolveDependencies(

return new Map(
dependencies.map(relativePath => [
relativePath,
dependencyGraph.resolveDependency(
parentModule,
relativePath,
transformOptions.platform,
).path,
relativePath,
]),
);
}
Expand Down

0 comments on commit 233b00e

Please sign in to comment.