Fix build cache management in workspaces #675
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| @@ -94,6 +94,14 @@ export function getOrCreateAppDownloadsDir() { | |||
| return downloadsDirLocation; | |||
| } | |||
|
|
|||
| export function getOrCreateBuildCachesDir() { | |||
There was a problem hiding this comment.
what do you think about creating abstract utils for app root level cache management? I believe it would be enough to support storing json data? It would be nice to use something like that in #640 to store recently opened deep links
on high level it could look something like this:
// src/utilities/appCache.ts
const readAppLevelCache = (key: string): Record<string, unknown> | null => {
return JSON.parse(fs.read(join(appCacheDir), key)));
}
const writeAppLevelCache = (key: string, data: Record<string, unknown>) => {
fs.write(join(appCacheDir, key), JSON.stringify(data));
}There was a problem hiding this comment.
You are right it would be great to it build like that, thank you
|
Is this really a scenario we want to optimize for? When I work with monorepo setups I never switch between opening the workspace from different places. I either have the app bit open, or I have the whole monorepo opened. I never switch between these two approaches for a given project. Do you have any insights whether this is what people tend to do? |
|
@kmagiera This PR as I eluded in description is necessary to enable switching between applications in #663, so the fact that it will help some people that like to change set up is just a bonus, but to answer your question when I talked to @jakex7 about his monorepo set up he seemed interested in being able to use booth approaches. |
| let oldCaches; | ||
| try { | ||
| oldCaches = JSON.parse(fs.readFileSync(appCachesPath).toString()); | ||
| } catch (e) { | ||
| Logger.warn(`Error parsing old caches file for app ${getAppRootFolder()}`, e); | ||
| return; | ||
| } |
There was a problem hiding this comment.
this logic is repeated several times, you can extract it to new function getAppCaches() or something like that (without key, it returns the whole json object)
| } | ||
|
|
||
| /** | ||
| * This function sets app level caches from caches storage. |
| @@ -135,7 +135,7 @@ export class BuildManager { | |||
| ); | |||
| } | |||
|
|
|||
| await buildCache.storeBuild(buildFingerprint, buildResult); | |||
| await buildCache.storeCache(buildFingerprint, buildResult); | |||
There was a problem hiding this comment.
I don't like the naming being so mixed up in this whole PR. We should have naming that better maps to the intuition what cache is rather than trying to provide a super-accurate naming.
Here, it sound very weird that we are "storing cache" using "build cache". Intuitively, what this line does is it stores some build metadata (build result) in a cache storage. IMO "storeBuild" just much better describes what this method does than "storeCache".
| @@ -53,21 +54,34 @@ export class PlatformBuildCache { | |||
| /** | |||
| * Passed fingerprint should be calculated at the time build is started. | |||
| */ | |||
| public async storeBuild(buildFingerprint: string, build: BuildResult) { | |||
| public async storeCache(buildFingerprint: string, build: BuildResult) { | |||
There was a problem hiding this comment.
why has this been renamed? The method takes fingerprint and build result that gets stored. We don't store any cache here
| fingerprint: buildFingerprint, | ||
| buildHash: appPath, | ||
| buildResult: build, | ||
| }); | ||
|
|
||
| setAppCache(this.cacheKey, cache); |
There was a problem hiding this comment.
In contrast, this method actually stores stuff, so why is it called "set" – this is a frequently used prefix for setting class members and using it for method that writes to disk is very confusing.
| * @param value | ||
| * @returns the new value that was set | ||
| */ | ||
| export function setAppCache(key: string, value: string) { |
There was a problem hiding this comment.
this whole module doesn't have a clear contract. It seem to assume that the values it saves or retrieves are strings, but internal implementation parses those files expecting this is actually a structured data
There was a problem hiding this comment.
I stores strings for given key but the whole caches file is a json
Co-authored-by: Katzper Michno <[email protected]>
…tware-mansion/react-native-ide into @Filip131311/FixFingerprintManagment
This PR changes a way we manage build caches. Previous approach was dependent on current vscode workspace, which led to build caches not being accessible when switching from working in the root of the workspace to the root of the application (or doing the same in reverse). The new approach makes sure of our caches directory and makes caches retrieval
appRootdependent instead of workspace dependent.To achieve this we introduce a new cacheing mechanism that can be accessed through:
getAppCache,setAppCacheandremoveAppCacheutilities.Finally we add a migration utility to extension activation process in other to make transition seamless for old users.
This PR is a dependency for #663
How Has This Been Tested: