chore(core): cache asset hashes #10478
Conversation
Cache asset hashes when staging. This avoids rebundling an asset with `AssetHashType.OUTPUT` when it is used in multiple stacks in an app. This also removes unnecessary file system operations for other asset hash types. Closes aws#9424
jogold
changed the title
| // Check if we already bundled this source path in this App (e.g. the same | ||
| // asset is used in multiple stacks). In this case we can completely skip | ||
| // file system and bundling operations. | ||
| this.cacheHash = crypto.createHash('sha256') |
There was a problem hiding this comment.
Maybe call this cacheKey, otherwise I get confused with all the occurrences of the word hash here.
Why does it need to be a member?
There was a problem hiding this comment.
Also feels like it would be cleaner if this calculation was in a function somewhere.
| this.assetHashCache = {}; | ||
| } | ||
|
|
||
| private static assetHashCache: { [key: string]: string } = {}; |
There was a problem hiding this comment.
Please describe in a comment what this does. It caches source hashes based on asset configuration.
Maybe also call it sourceHashCache?
There was a problem hiding this comment.
The name sourceHash is deprecated in favor of assetHash so I propose to stick with assetHashCache.
There was a problem hiding this comment.
Heh, I missed that. Alrighty.
| // file system and bundling operations. | ||
| this.cacheHash = crypto.createHash('sha256') | ||
| .update(path.resolve(this.sourcePath)) | ||
| .update(JSON.stringify(props.bundling)) |
There was a problem hiding this comment.
Has props.bundling been normalized yet? It probably needs to be, otherwise 2 sets of properties that morally represent the same asset configuration render to 2 different keys (I'm thinking for example of { packaging } specified or omitted because of the default)
There was a problem hiding this comment.
"Normalized" it by recursively sorting the keys after setting some defaults in the props.
| .update(path.resolve(this.sourcePath)) | ||
| .update(JSON.stringify(props.bundling)) | ||
| .digest('hex'); | ||
| if (AssetStaging.assetHashCache[this.cacheHash]) { |
There was a problem hiding this comment.
I think a small helper function makes this slightly nicer:
this.assetHash = AssetStaging.cachedSourceHash(cacheKey, () => {
// ... calculation here...
});(Or think of a nice other name)
mergify
bot
dismissed
rix0rrr’s stale review
Pull request has been modified.
jogold
changed the title
|
Changed to cache asset hash also when not bundling (avoids re-fingerprinting already fingerprinted dirs). |
|
This (weird) test is now failing: In this test the content of an asset is changed with a @rix0rrr what do you say? |
It doesn't though. It also tests that the hash is reflected in I think I would have preferred writing the same files to different asset locations (to avoid the cache), but the way you fixed it is fine too. |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Cache asset hashes. This avoids file system and bundling operations
when the same asset with the same configuration is used in multiple
stacks in an app.
Closes #9424
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license