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

Use content-hash to invalidate cache #1006

Merged
merged 4 commits into from
Nov 24, 2020
Merged

Use content-hash to invalidate cache #1006

merged 4 commits into from
Nov 24, 2020

Conversation

fvictorio
Copy link
Member

No description provided.

throw new Error("Couldn't read cache file, try running the clean task"); // TODO use HardhatError
log("There was a problem reading the cache");

return new SolidityFilesCache({
Copy link
Member

Choose a reason for hiding this comment

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

This will clean the cache as soon as the user upgrades, right?

Copy link
Member

Choose a reason for hiding this comment

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

I mean, because of the change in the cache file format.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I forgot to add a comment about this.

I think in an ideal world we would handle changes in the files format in a graceful way. For example: detecting that the file has the format v1 but the code expects format v2, and acting accordingly. But in this case, just ignoring everything and starting over is both easier and (I think) correct. We might need to change this approach in the future, but for now I think this is enough.

Copy link
Member

Choose a reason for hiding this comment

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

agreed

contentHash: string
): ParsedData | null {
if (this._solidityFilesCache === undefined) {
return this._cache.get(absolutePath) ?? null;
Copy link
Member

Choose a reason for hiding this comment

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

What if we use the content hash instead of the absolute path here? I think it will be more robust. I can imagine using the absolutePath bringing issues for things like the hardhat-watcher plugin.

const cacheEntry = this._solidityFilesCache.getEntry(absolutePath);

if (cacheEntry === undefined) {
return this._cache.get(absolutePath) ?? null;
Copy link
Member

Choose a reason for hiding this comment

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

Same

Copy link
Member

@alcuadrado alcuadrado left a comment

Choose a reason for hiding this comment

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

This turned out to be a very elegant solution.

The only thing I'd change is the Parser's in-memory cache.

@fvictorio
Copy link
Member Author

The only thing I'd change is the Parser's in-memory cache.

You mean we keep the [absolutePath: string]: CacheEntry structure, but we use the hash for indexing only for the in memory object that represents the cache?

@alcuadrado alcuadrado merged commit bb53120 into master Nov 24, 2020
@alcuadrado alcuadrado deleted the content-hash-cache branch November 24, 2020 18:42
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants