-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,8 +14,12 @@ export class Parser { | |
|
||
constructor(private _solidityFilesCache?: SolidityFilesCache) {} | ||
|
||
public parse(fileContent: string, absolutePath: string): ParsedData { | ||
const cacheResult = this._getFromCache(absolutePath); | ||
public parse( | ||
fileContent: string, | ||
absolutePath: string, | ||
contentHash: string | ||
): ParsedData { | ||
const cacheResult = this._getFromCache(absolutePath, contentHash); | ||
|
||
if (cacheResult !== null) { | ||
return cacheResult; | ||
|
@@ -55,12 +59,28 @@ export class Parser { | |
return result; | ||
} | ||
|
||
private _getFromCache(absolutePath: string): ParsedData | null { | ||
const cacheEntry = this._solidityFilesCache?.getEntry(absolutePath); | ||
/** | ||
* Get parsed data from the internal cache, or from the solidity files cache. | ||
* | ||
* Returns null if cannot find it in either one. | ||
*/ | ||
private _getFromCache( | ||
absolutePath: string, | ||
contentHash: string | ||
): ParsedData | null { | ||
if (this._solidityFilesCache === undefined) { | ||
return this._cache.get(absolutePath) ?? null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} | ||
|
||
const cacheEntry = this._solidityFilesCache.getEntry(absolutePath); | ||
|
||
if (cacheEntry === undefined) { | ||
return this._cache.get(absolutePath) ?? null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same |
||
} | ||
|
||
if (cacheEntry !== undefined) { | ||
const { imports, versionPragmas } = cacheEntry; | ||
const { imports, versionPragmas } = cacheEntry; | ||
|
||
if (cacheEntry.contentHash === contentHash) { | ||
return { imports, versionPragmas }; | ||
} | ||
|
||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed