Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reflect child bundle contents in own bundle hash #3414

Merged
merged 4 commits into from
Aug 22, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 27 additions & 3 deletions packages/core/core/src/BundleGraph.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ type BundleGraphEdgeTypes =
| 'references';

export default class BundleGraph {
// A cache of bundle content hashes. Currently, a new BundleGraph is created in response
// to any asset change, so this doesn't need much invalidation. However, currently namers run
// before runtimes, and can access `getHash` despite runtimes altering bundle content later.
// TODO: Implement invalidation since runtimes can alter bundle contents?
_bundleContentHashes: Map<string, string> = new Map();
_graph: Graph<BundleGraphNode, BundleGraphEdgeTypes>;

constructor(graph: Graph<BundleGraphNode, BundleGraphEdgeTypes>) {
Expand Down Expand Up @@ -282,11 +287,14 @@ export default class BundleGraph {
return this._graph.getNodesConnectedFrom(bundleNode, 'bundle').length > 0;
}

traverseBundles<TContext>(visit: GraphVisitor<Bundle, TContext>): ?TContext {
traverseBundles<TContext>(
visit: GraphVisitor<Bundle, TContext>,
startBundle?: Bundle
): ?TContext {
return this._graph.filteredTraverse(
node => (node.type === 'bundle' ? node.value : null),
visit,
null,
startBundle ? nullthrows(this._graph.getNode(startBundle.id)) : null,
'bundle'
);
}
Expand Down Expand Up @@ -455,13 +463,29 @@ export default class BundleGraph {
return {asset, exportSymbol: symbol, symbol: identifier};
}

getHash(bundle: Bundle): string {
getContentHash(bundle: Bundle): string {
let existingHash = this._bundleContentHashes.get(bundle.id);
if (existingHash != null) {
return existingHash;
}

let hash = crypto.createHash('md5');
// TODO: sort??
this.traverseAssets(bundle, asset => {
hash.update(asset.outputHash);
});

let hashHex = hash.digest('hex');
this._bundleContentHashes.set(bundle.id, hashHex);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since computing the hash for a bundle how involves computing content hashes for its entire bundle subgraph (in cases where there's only one entry, this is every bundle), cache them.

return hashHex;
}

getHash(bundle: Bundle): string {
let hash = crypto.createHash('md5');
this.traverseBundles(childBundle => {
hash.update(this.getContentHash(childBundle));
}, bundle);

return hash.digest('hex');
}
}
66 changes: 35 additions & 31 deletions packages/core/integration-tests/test/contentHashing.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,19 @@
const assert = require('assert');
const path = require('path');
const {bundle, rimraf, ncp, inputFS: fs} = require('@parcel/test-utils');
import assert from 'assert';
import path from 'path';
import {bundle as _bundle, outputFS, ncp} from '@parcel/test-utils';

describe.skip('content hashing', function() {
beforeEach(async function() {
await rimraf(path.join(__dirname, '/input'));
function bundle(path) {
return _bundle(path, {
inputFS: outputFS,
disableCache: false
});
}

const distDir = '/dist';

describe('content hashing', function() {
beforeEach(async () => {
await outputFS.rimraf(path.join(__dirname, '/input'));
});

it('should update content hash when content changes', async function() {
Expand All @@ -13,33 +22,29 @@ describe.skip('content hashing', function() {
path.join(__dirname, '/input')
);

await bundle(path.join(__dirname, '/input/index.html'), {
production: true
});
let bundleHtml = () => bundle(path.join(__dirname, '/input/index.html'));
await bundleHtml();

let html = await fs.readFile(
path.join(__dirname, '/dist/index.html'),
let html = await outputFS.readFile(
path.join(distDir, 'index.html'),
'utf8'
);
let filename = html.match(
/<link rel="stylesheet" href="[/\\]{1}(input\.[a-f0-9]+\.css)">/
)[1];
assert(await fs.exists(path.join(__dirname, '/dist/', filename)));
assert(await outputFS.exists(path.join(distDir, filename)));

await fs.writeFile(
await outputFS.writeFile(
path.join(__dirname, '/input/index.css'),
'body { background: green }'
);
await bundleHtml();

await bundle(path.join(__dirname, '/input/index.html'), {
production: true
});

html = await fs.readFile(path.join(__dirname, '/dist/index.html'), 'utf8');
html = await outputFS.readFile(path.join(distDir, 'index.html'), 'utf8');
let newFilename = html.match(
/<link rel="stylesheet" href="[/\\]{1}(input\.[a-f0-9]+\.css)">/
)[1];
assert(await fs.exists(path.join(__dirname, '/dist/', newFilename)));
assert(await outputFS.exists(path.join(distDir, newFilename)));

assert.notEqual(filename, newFilename);
});
Expand All @@ -50,23 +55,22 @@ describe.skip('content hashing', function() {
path.join(__dirname, '/input')
);

await bundle(path.join(__dirname, '/input/index.js'), {
production: true
});
let bundleJs = () => bundle(path.join(__dirname, '/input/index.js'));
await bundleJs();

let js = await fs.readFile(path.join(__dirname, '/dist/index.js'), 'utf8');
let js = await outputFS.readFile(path.join(distDir, 'index.js'), 'utf8');
let filename = js.match(/\/(test\.[0-9a-f]+\.txt)/)[1];
assert(await fs.exists(path.join(__dirname, '/dist/', filename)));

await fs.writeFile(path.join(__dirname, '/input/test.txt'), 'hello world');
assert(await outputFS.exists(path.join(distDir, filename)));

await bundle(path.join(__dirname, '/input/index.js'), {
production: true
});
await outputFS.writeFile(
path.join(__dirname, '/input/test.txt'),
'hello world'
);
await bundleJs();

js = await fs.readFile(path.join(__dirname, '/dist/index.js'), 'utf8');
js = await outputFS.readFile(path.join(distDir, 'index.js'), 'utf8');
let newFilename = js.match(/\/(test\.[0-9a-f]+\.txt)/)[1];
assert(await fs.exists(path.join(__dirname, '/dist/', newFilename)));
assert(await outputFS.exists(path.join(distDir, newFilename)));

assert.notEqual(filename, newFilename);
});
Expand Down
1 change: 0 additions & 1 deletion packages/core/test-utils/src/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ export async function ncp(source: FilePath, destination: FilePath) {
.on('error', reject);
});
} else if (stats.isDirectory()) {
outputFS.mkdirp(destPath);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't awaited, and it also happens anyway in the next recursive call to ncp.

await ncp(sourcePath, destPath);
}
}
Expand Down