feat: Improve rebuild of native plugins and node_modules checks #3750
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently all changes in `node_modules` are skipped as we do not check for changes in case `--syncAllFiles` is not passed. In fact the meaning of this option is for LiveSync operations and by default CLI does not watch for changes in node_modules. However, the separate commands like prepare, build, test and even the initial LiveSync must check the node_modules and prepare the correct files. In latest versions (4.2.x) we were always checking all `node_modules` even during change of a single file during LiveSync. After that we've applied a change in the current master branch to skip node_modules check in case specific option is passed to the projectChangesService. The problem with latest approach is that we've missed this option is always passed based on syncAllFiles. This led to the problem that once you have node_modules setup, CLI from master branch never checks for changes in these files and does not move them to platforms dir. With the current changes, node_modules will be checked on all commands. Once LiveSync starts, all consecutive changes in project files will not check node_modules unless `syncAllFiles` option is passed to CLI.
In case CLI builds `.aar` file for the Android part of a plugin during project preparation, consecutive prepares should not build the `.aar` file. Add logic to keep the hashes of the files that have been used for building the plugin and persist them in the `<project dir>/platforms/tempPlugin/<plugin-name>/plugin-data.json`. Before building a new `.aar` file, CLI will check if the mentioned file exists and compare its content with the shasums of the current source files of the plugin. This way we ensure the `.aar` will be built only when the sources are changed.
In case a plugin needs to apply some modifications over its own source, based on the project files, currently it cannot do it. The earliest possible hooks are before-shouldPrepare and beforePrepare, but they are both run after CLI had check the project for changes. In case the plugin uses any of these hooks and apply changes to its source code, CLI will not move the changes to platforms dir of the project as it will not recheck the project state. In order to resolve this issue, add a hook to checkForChanges method. This way a plugin can apply the changes in before-checkForChanges hook and CLI will detect them correctly. Change the method parameters to be a single object, which will allow easier modifications in the future without breaking the plugins that are already using the hook.
rosen-vladimirov
force-pushed
the
vladimirov/rebuild-plugin-when-required-only
branch
from
0eca517 to
86a7e55
Compare
Fatme
reviewed
Jul 16, 2018
lib/definitions/project-changes.d.ts
Outdated
| } | ||
|
|
||
| interface ICheckForChangesOptions extends IPlatform, IProjectDataComposition { | ||
| projectChangesOptions: IProjectChangesOptions |
There was a problem hiding this comment.
missing semicolon at the end.
| return this.$filesHashService.generateHashes(pluginNativeDataFiles); | ||
| } | ||
|
|
||
| private async writePluginHashInfo(fileHashesInfo: IStringDictionary, pluginTempDir: string): Promise<void> { |
There was a problem hiding this comment.
No need to be async method.
Also return type should be void
| fileHashesInfo: IStringDictionary | ||
| }): Promise<boolean> { | ||
|
|
||
| let shouldBuildAar = !!opts.manifestFilePath || opts.androidSourceDirectories.length > 0; |
There was a problem hiding this comment.
!!opts.manifestFilePath || opts.androidSourceDirectories.length
should be enough check
There was a problem hiding this comment.
In this way shouldBuildAar will become integer instead of boolean. I suppose that even the tslint check will fail as the method should return boolean. I believe that we should not change the variable types based on some conditions. If we want this one shorter, we could use let shouldBuildAar = !!opts.manifestFilePath || !!opts.androidSourceDirectories.length; if you believe that its more readable.
|
|
||
| private getSourceFilesHashes(pluginTempPlatformsAndroidDir: string, shortPluginName: string): Promise<IStringDictionary> { | ||
| const pathToAar = path.join(pluginTempPlatformsAndroidDir, `${shortPluginName}.aar`); | ||
| const pluginNativeDataFiles = this.$fs.enumerateFilesInDirectorySync(pluginTempPlatformsAndroidDir, (file: string, stat: IFsStats) => { |
There was a problem hiding this comment.
this.$fs.enumerateFilesInDirectorySync(pluginTempPlatformsAndroidDir, (file: string, stat: IFsStats) => file !== pathToAar);
is shorter
DimitarTachev
approved these changes
Jul 16, 2018
| const shortPluginName = getShortPluginName(options.pluginName); | ||
| const pluginTempDir = path.join(options.tempPluginDirPath, shortPluginName); | ||
| const pluginTempMainSrcDir = path.join(pluginTempDir, "src", "main"); | ||
| // In case plugin was already built in the current process, we need to clean the old sources as they may break the new build. |
There was a problem hiding this comment.
move these 3 lines in something like this.clean(pluginTempDir) or even fs.clean in order to keep the method as short and readable as possible
| fileHashesInfo: IStringDictionary | ||
| }): Promise<boolean> { | ||
|
|
||
| let shouldBuildAar = !!opts.manifestFilePath || opts.androidSourceDirectories.length > 0; |
There was a problem hiding this comment.
In this way shouldBuildAar will become integer instead of boolean. I suppose that even the tslint check will fail as the method should return boolean. I believe that we should not change the variable types based on some conditions. If we want this one shorter, we could use let shouldBuildAar = !!opts.manifestFilePath || !!opts.androidSourceDirectories.length; if you believe that its more readable.
|
run ci |
rosen-vladimirov
deleted the
vladimirov/rebuild-plugin-when-required-only
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fix: Changes in node_modules are always skipped
Currently all changes in
node_modulesare skipped as we do not check for changes in case--syncAllFilesis not passed.In fact the meaning of this option is for LiveSync operations and by default CLI does not watch for changes in node_modules. However, the separate commands like prepare, build, test and even the initial LiveSync must check the node_modules and prepare the correct files.
In latest versions (4.1.x) we were always checking all
node_moduleseven during change of a single file during LiveSync. After that we've applied a change in the current master branch to skip node_modules check in case specific option is passed to the projectChangesService. The problem with latest approach is that we've missed this option is always passed based on syncAllFiles.This led to the problem that once you have node_modules setup, CLI from master branch never checks for changes in these files and does not move them to platforms dir.
With the current changes, node_modules will be checked on all commands. Once LiveSync starts, all consecutive changes in project files will not check node_modules unless
syncAllFilesoption is passed to CLI.feat: Rebuild plugins for Android only when necessary
In case CLI builds
.aarfile for the Android part of a plugin during project preparation, consecutive prepares should not build the.aarfile. Add logic to keep the hashes of the files that have been used for building the plugin and persist them in the<project dir>/platforms/tempPlugin/<plugin-name>/plugin-data.json.Before building a new
.aarfile, CLI will check if the mentioned file exists and compare its content with the shasums of the current source files of the plugin. This way we ensure the.aarwill be built only when the sources are changed.feat: Add hook for checkForChanges
In case a plugin needs to apply some modifications over its own source, based on the project files, currently it cannot do it. The earliest possible hooks are before-shouldPrepare and beforePrepare, but they are both run after CLI had check the project for changes. In case the plugin uses any of these hooks and apply changes to its source code, CLI will not move the changes to platforms dir of the project as it will not recheck the project state.
In order to resolve this issue, add a hook to checkForChanges method. This way a plugin can apply the changes in before-checkForChanges hook and CLI will detect them correctly.
Change the method parameters to be a single object, which will allow easier modifications in the future without breaking the plugins that are already using the hook.
PR Checklist
What is the current behavior?
CLI 4.1.x:
Whenever CLI checks for changes, it always lists all node_modules and checks for changes there. This happens on all commands and on every change during LiveSync. In case a change in
node_modulesis detected, CLI checks if it is inplatforms/androiddirectory of the plugin. In case yes, CLI rebuilds all plugins which do not have their own.aar. CLI rebuilds the.aarfiles even for plugins, for which it has already built.aarand there's no change in their source code that requires rebuild of this.aar.master branch
CLI skips all checks for changes in
node_modules. The only chance to get your changes applied is to pass--syncAllFiles. Whenever a change inplatforms/androiddir of any plugin is detected and--syncAllFilesis passed, CLI will rebuild all.aarfiles.What is the new behavior?
CLI checks
node_modulesfor changes during all commands. During LiveSync, CLI will checknode_modulesfor changes only during the initial sync (i.e. the first operation of the started command). When a change is detected, CLI will not checknode_modulesfor changes unless--syncAllFilesis passed.When a change in any
platforms/androiddirectory of a plugin is detected, CLI will check all plugins and their source files. In case the source files that are used for building the.aarhave not been changed since last build, CLI will not rebuild the.aar.Fixes/Implements/Closes #[Issue Number].