Merge pull request #2904 from parcel-bundler/wbinnssmith/no-duplicate…

…-connectedfiles

Make connectedFiles and dependencies Maps

packages/core/core/src/Asset.js

@@ -27,24 +27,32 @@ type AssetOptions = {|
   type: string,
   code?: string,
   ast?: ?AST,
-  dependencies?: Array<IDependency>,
-  connectedFiles?: Array<File>,
+  dependencies?: Iterable<[string, IDependency]>,
+  connectedFiles?: Iterable<[FilePath, File]>,
   output?: AssetOutput,
   outputHash?: string,
   env: Environment,
   meta?: Meta,
   stats?: Stats
 |};
 
+type SerializedOptions = {|
+  ...AssetOptions,
+  ...{|
+    connectedFiles: Array<[FilePath, File]>,
+    dependencies: Array<[string, IDependency]>
+  |}
+|};
+
 export default class Asset implements IAsset {
   id: string;
   hash: string;
   filePath: FilePath;
   type: string;
   code: string;
   ast: ?AST;
-  dependencies: Array<IDependency>;
-  connectedFiles: Array<File>;
+  dependencies: Map<string, IDependency>;
+  connectedFiles: Map<FilePath, File>;
   output: AssetOutput;
   outputHash: string;
   env: Environment;
@@ -63,11 +71,11 @@ export default class Asset implements IAsset {
     this.code = options.code || (options.output ? options.output.code : '');
     this.ast = options.ast || null;
     this.dependencies = options.dependencies
-      ? options.dependencies.slice()
-      : [];
+      ? new Map(options.dependencies)
+      : new Map();
     this.connectedFiles = options.connectedFiles
-      ? options.connectedFiles.slice()
-      : [];
+      ? new Map(options.connectedFiles)
+      : new Map();
     this.output = options.output || {code: this.code};
     this.outputHash = options.outputHash || '';
     this.env = options.env;
@@ -78,15 +86,15 @@ export default class Asset implements IAsset {
     };
   }
 
-  serialize(): AssetOptions {
+  serialize(): SerializedOptions {
     // Exclude `code` and `ast` from cache
     return {
       id: this.id,
       hash: this.hash,
       filePath: this.filePath,
       type: this.type,
-      dependencies: this.dependencies,
-      connectedFiles: this.connectedFiles,
+      dependencies: Array.from(this.dependencies),
+      connectedFiles: Array.from(this.connectedFiles),
       output: this.output,
       outputHash: this.outputHash,
       env: this.env,
@@ -96,13 +104,14 @@ export default class Asset implements IAsset {
   }
 
   addDependency(opts: DependencyOptions) {
-    // $FlowFixMe
+    let {env, ...rest} = opts;
     let dep = new Dependency({
-      ...opts,
-      env: this.env.merge(opts.env),
+      ...rest,
+      env: this.env.merge(env),
       sourcePath: this.filePath
     });
-    this.dependencies.push(dep);
+
+    this.dependencies.set(dep.id, dep);
     return dep.id;
   }
 
@@ -111,7 +120,15 @@ export default class Asset implements IAsset {
       file.hash = await md5FromFilePath(file.filePath);
     }
 
-    this.connectedFiles.push(file);
+    this.connectedFiles.set(file.filePath, file);
+  }
+
+  getConnectedFiles(): Array<File> {
+    return Array.from(this.connectedFiles.values());
+  }
+
+  getDependencies(): Array<IDependency> {
+    return Array.from(this.dependencies.values());
   }
 
   createChildAsset(result: TransformerResult) {
@@ -131,14 +148,16 @@ export default class Asset implements IAsset {
 
     let asset = new Asset(opts);
 
-    if (result.dependencies) {
-      for (let dep of result.dependencies) {
+    let dependencies = result.dependencies;
+    if (dependencies) {
+      for (let dep of dependencies.values()) {
         asset.addDependency(dep);
       }
     }
 
-    if (result.connectedFiles) {
-      for (let file of result.connectedFiles) {
+    let connectedFiles = result.connectedFiles;
+    if (connectedFiles) {
+      for (let file of connectedFiles.values()) {
         asset.addConnectedFile(file);
       }
     }

packages/core/core/src/AssetGraph.js

@@ -189,7 +189,7 @@ export default class AssetGraph extends Graph<AssetGraphNode>
     // Get connected files from each asset and connect them to the file node
     let fileNodes = [];
     for (let asset of cacheEntry.assets) {
-      let files = asset.connectedFiles.map(file => nodeFromFile(file));
+      let files = asset.getConnectedFiles().map(file => nodeFromFile(file));
       fileNodes = fileNodes.concat(files);
     }
 
@@ -206,9 +206,9 @@ export default class AssetGraph extends Graph<AssetGraphNode>
     let removedFiles = getFilesFromGraph(removed);
 
     for (let assetNode of assetNodes) {
-      let depNodes = assetNode.value.dependencies.map(dep => {
-        return nodeFromDep(dep);
-      });
+      let depNodes = assetNode.value
+        .getDependencies()
+        .map(dep => nodeFromDep(dep));
       let {removed, added} = this.replaceNodesConnectedTo(assetNode, depNodes);
       removedFiles = removedFiles.concat(getFilesFromGraph(removed));
       newDepNodes = newDepNodes.concat(getDepNodesFromGraph(added));

packages/core/core/src/Dependency.js

@@ -13,7 +13,6 @@ import {md5FromString} from '@parcel/utils/src/md5';
 
 type DependencyOpts = {|
   ...DependencyOptions,
-  moduleSpecifier: ModuleSpecifier,
   env: IEnvironment,
   id?: string,
   sourcePath?: FilePath

packages/core/core/src/TransformerRunner.js

@@ -250,7 +250,7 @@ async function finalize(asset: Asset, generate: GenerateFunc): Promise<Asset> {
 
 async function checkCachedAssets(assets: Array<IAsset>): Promise<boolean> {
   let results = await Promise.all(
-    assets.map(asset => checkConnectedFiles(asset.connectedFiles))
+    assets.map(asset => checkConnectedFiles(asset.getConnectedFiles()))
   );
 
   return results.every(Boolean);

packages/core/core/test/Asset.test.js

@@ -0,0 +1,53 @@
+// @flow strict-local
+
+import assert from 'assert';
+import Asset from '../src/Asset';
+import Environment from '../src/Environment';
+
+describe('Asset', () => {
+  it('only includes connected files once per filePath', () => {
+    let asset = new Asset({
+      filePath: '/foo/asset.js',
+      env: new Environment(),
+      type: 'js'
+    });
+    asset.addConnectedFile({filePath: '/foo/file', hash: 'abc'});
+    asset.addConnectedFile({filePath: '/foo/file', hash: 'bcd'});
+    assert.deepEqual(asset.getConnectedFiles(), [
+      {
+        filePath: '/foo/file',
+        hash: 'bcd'
+      }
+    ]);
+  });
+
+  it('only includes dependencies once per id', () => {
+    let asset = new Asset({
+      filePath: '/foo/asset.js',
+      env: new Environment(),
+      type: 'js'
+    });
+
+    asset.addDependency({moduleSpecifier: './foo'});
+    asset.addDependency({moduleSpecifier: './foo'});
+    let dependencies = asset.getDependencies();
+    assert(dependencies.length === 1);
+    assert(dependencies[0].moduleSpecifier === './foo');
+  });
+
+  it('includes different dependencies if their id differs', () => {
+    let asset = new Asset({
+      filePath: '/foo/asset.js',
+      env: new Environment(),
+      type: 'js'
+    });
+
+    asset.addDependency({moduleSpecifier: './foo'});
+    asset.addDependency({
+      moduleSpecifier: './foo',
+      env: {context: 'web-worker', engines: {}}
+    });
+    let dependencies = asset.getDependencies();
+    assert(dependencies.length === 2);
+  });
+});

packages/core/core/test/AssetGraph.test.js

@@ -146,11 +146,14 @@ describe('AssetGraph', () => {
         type: 'js',
         hash: '#1',
         dependencies: [
-          new Dependency({
-            moduleSpecifier: './utils',
-            env: DEFAULT_ENV,
-            sourcePath
-          })
+          [
+            'utils',
+            new Dependency({
+              moduleSpecifier: './utils',
+              env: DEFAULT_ENV,
+              sourcePath
+            })
+          ]
         ],
         env: DEFAULT_ENV,
         output: {code: ''},
@@ -162,11 +165,14 @@ describe('AssetGraph', () => {
         type: 'js',
         hash: '#2',
         dependencies: [
-          new Dependency({
-            moduleSpecifier: './styles',
-            env: DEFAULT_ENV,
-            sourcePath
-          })
+          [
+            'styles',
+            new Dependency({
+              moduleSpecifier: './styles',
+              env: DEFAULT_ENV,
+              sourcePath
+            })
+          ]
         ],
         env: DEFAULT_ENV,
         output: {code: ''},
@@ -196,8 +202,8 @@ describe('AssetGraph', () => {
     assert(graph.nodes.has('1'));
     assert(graph.nodes.has('2'));
     assert(graph.nodes.has('3'));
-    assert(graph.nodes.has(assets[0].dependencies[0].id));
-    assert(graph.nodes.has(assets[1].dependencies[0].id));
+    assert(graph.nodes.has(assets[0].getDependencies()[0].id));
+    assert(graph.nodes.has(assets[1].getDependencies()[0].id));
     assert(graph.nodes.has('/index.js'));
     assert(
       graph.hasEdge({
@@ -226,13 +232,13 @@ describe('AssetGraph', () => {
     assert(
       graph.hasEdge({
         from: '1',
-        to: assets[0].dependencies[0].id
+        to: assets[0].getDependencies()[0].id
       })
     );
     assert(
       graph.hasEdge({
         from: '2',
-        to: assets[1].dependencies[0].id
+        to: assets[1].getDependencies()[0].id
       })
     );
     assert(!graph.incompleteNodes.has(nodeFromTransformerRequest(req).id));
@@ -262,11 +268,14 @@ describe('AssetGraph', () => {
         type: 'js',
         hash: '#1',
         dependencies: [
-          new Dependency({
-            moduleSpecifier: './utils',
-            env: DEFAULT_ENV,
-            sourcePath
-          })
+          [
+            'utils',
+            new Dependency({
+              moduleSpecifier: './utils',
+              env: DEFAULT_ENV,
+              sourcePath
+            })
+          ]
         ],
         env: DEFAULT_ENV,
         output: {code: ''},
@@ -296,8 +305,8 @@ describe('AssetGraph', () => {
     assert(graph.nodes.has('1'));
     assert(graph.nodes.has('2'));
     assert(!graph.nodes.has('3'));
-    assert(graph.nodes.has(assets[0].dependencies[0].id));
-    assert(!graph.nodes.has(assets[1].dependencies[0].id));
+    assert(graph.nodes.has(assets[0].getDependencies()[0].id));
+    assert(!graph.nodes.has(assets[1].getDependencies()[0].id));
     assert(
       graph.hasEdge({
         from: nodeFromTransformerRequest(req).id,
@@ -325,13 +334,13 @@ describe('AssetGraph', () => {
     assert(
       graph.hasEdge({
         from: '1',
-        to: assets[0].dependencies[0].id
+        to: assets[0].getDependencies()[0].id
       })
     );
     assert(
       !graph.hasEdge({
         from: '2',
-        to: assets[1].dependencies[0].id
+        to: assets[1].getDependencies()[0].id
       })
     );
     assert(!graph.incompleteNodes.has(nodeFromTransformerRequest(req).id));
@@ -375,19 +384,25 @@ describe('AssetGraph', () => {
         type: 'js',
         hash: '#1',
         dependencies: [
-          new Dependency({
-            moduleSpecifier: './utils',
-            env: DEFAULT_ENV,
-            sourcePath
-          })
+          [
+            'utils',
+            new Dependency({
+              moduleSpecifier: './utils',
+              env: DEFAULT_ENV,
+              sourcePath
+            })
+          ]
         ],
         env: DEFAULT_ENV,
         output: {code: ''},
-        connectedFiles: [
-          {
-            filePath: '/foo/bar'
-          }
-        ]
+        connectedFiles: new Map([
+          [
+            '/foo/bar',
+            {
+              filePath: '/foo/bar'
+            }
+          ]
+        ])
       })
     ];
     let cacheEntry = {