Add custom fingerprinting and build cache #589
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| private checkIfNativeChanged = throttle(async (cancelToken: CancelToken) => { | ||
| if (!this.buildCacheInvalidated && this.projectState.selectedDevice) { | ||
| // TODO: should be refactored to not create a new PlatformBuildCache | ||
| // instance every time | ||
| const platform = this.projectState.selectedDevice.platform; | ||
| const buildCache = new PlatformBuildCache(platform, cancelToken); | ||
|
|
||
| if (await buildCache.isCacheInvalidated()) { | ||
| this.buildCacheInvalidated = true; | ||
| this.eventEmitter.emit("needsNativeRebuild"); | ||
| } | ||
| } | ||
| }, 300); |
There was a problem hiding this comment.
Build cache shouldn't live here but I considered few options and all of them made structure more complex.
| buildResult, | ||
| }); | ||
|
|
||
| await buildCache.storeBuild(buildResult); |
There was a problem hiding this comment.
The issue here is that we actually want to use the fingerprint from before the build. Since builds take quite some time, there's a chance that someone will update some files that'd impact the fingerprint, in which case it'd be better to run another build afterwards than think that the build reflects all the changes done in the meantime.
| } | ||
| } | ||
|
|
||
| public async isCacheInvalidated() { |
There was a problem hiding this comment.
"invalidating" here is not the right term. It refers to the fact of cleaning the cached data. In this case, it'd be checking if the stored cache key still exists. This method however, checks whether the stored cache is up to date. I'd rename to hasUpToDateBuild or something along these lines
There was a problem hiding this comment.
This inverts logic – this was originally named detectedFingerprintChange. I renamed the method to isCacheStale, keeping the original boolean direction.
|
|
||
| constructor( | ||
| private readonly platform: DevicePlatform, | ||
| private readonly cancelToken: CancelToken |
There was a problem hiding this comment.
cancel tokens are transient and shouldn't be kept like that. Their purpose is to be able to pass them to async methods, such that they can be intercepted. Storing cancelToken like that defeats this purpose, because who is now in the control of cancelling it?
We should likely pass cancelTokens to methods that needs them directly, such that the caller is responsible for cancelling.
Alternatively we can create new cancel tokens for the calls that require them and expose some cancel method from this class which may be called by the caller.
| // TODO: should be refactored to not create a new PlatformBuildCache | ||
| // instance every time | ||
| const platform = this.projectState.selectedDevice.platform; | ||
| const buildCache = new PlatformBuildCache(platform, cancelToken); |
There was a problem hiding this comment.
responding to your comment here: maybe we could have a static method in PlatformBuildCache.get(platform) that returns an instance. This way we can hide the fact it is a stateless object and avoid constructing it in multiple places
There was a problem hiding this comment.
This works quite nicely but we now split getting fingerprint and storing the build (which is correct) so the value of the class is reduced. I think it's still marginally better than standalone functions but not by much.
…of cache We need to store fingerprint that was calculated at the start of the build process instead of calculating it after it finishes, as user can modify workspace while build is happening. Cancel token was removed because we expect fingerprints to be calculated much quicker than it takes to build the app.
| the last line of the standard output. If custom fingerprint script is used, it | ||
| should output fingerprint as the last line of the standard output. When | ||
| fingerprint changes between invocations, RN IDE will rebuild the project. The | ||
| IDE runs fingerprint quite frequently (i.e., on every file save), so this | ||
| process should be fast and avoid over the network communication. |
There was a problem hiding this comment.
We should explain the concept of fingerprint here. Specifically that it is a string that allows us to tell whether new native build needs to be run
| "buildScript": { | ||
| "android": "npm run build:ftp-fetch-android" | ||
| "customBuild": { | ||
| "android": { "buildScript": "npm run build:ftp-fetch-android" } |
There was a problem hiding this comment.
This isn't a script, so maybe we should rename to buildCommand? On the other hand in package.json you also have scripts sections where you can put commands and not just scripts.
| // TODO: should be refactored to not create a new PlatformBuildCache | ||
| // instance every time |
There was a problem hiding this comment.
Is this still relevant? We're not creating it here directly now
buildScriptin configuration withcustomBuildobject that allows using custom build and fingerprinting scriptsTesting
Checked force rebuilds, closing IDE while building, and detecting changed fingerprints when file change for regressions.
Checked custom fingerprints when closing and opening IDE and when project files change (installing new native dependency, changing fingerprint and saving file).