[FEATURE] clear build folder#216
[FEATURE] clear build folder#216mauriciolauffer wants to merge 5 commits intoSAP:masterfrom mauriciolauffer:clear-build-folder
Conversation
…building a project Fixes: #45
|
I would rather not always clean the dist directory. There can be cases where you just want to overwrite specific files with a smaller and faster build (i.e. updating some files without re-doing the whole build). An example for this might be the new JSDoc build and how it is used in the OpenUI5 testsuite: You have to execute the full build one time. After that only a subset, of the build tasks is required to update the JSDoc. This speeds up the build time by about one minute in this example. So maybe we could add an option to activate cleaning the dist directory before build? In that case I would also rather locate this in the UI5 CLI than in the UI5 Builder project, since I would not expect a function that is just called |
|
Hi @RandomByte. I've defined it as a parameter, false by default. I've added a test case for it. Also, I've moved rimraf from devDependencies to dependencies. |
|
I'm not sure whether it should be a command or an option in ui5-cli. I'm more inclined to use it as an option... What do you think is better? |
|
PS: I tried to create it as a task, but I couldn't find a way to pass the destPath as parameter to the task without changing too many files... |
RandomByte
left a comment
There was a problem hiding this comment.
Thanks for the update. I think @tommyvinhlam and @stopcoder should have a look into this PR as well. But here's my feedback for now.
PS: I tried to create it as a task, but I couldn't find a way to pass the destPath as parameter to the task without changing too many files...
Thats ok. Tasks are meant to process project resources and work on our internal virtual file system. Only the builder itself should care about the physical destination directory.
RandomByte
left a comment
There was a problem hiding this comment.
Some additional change requests than came up during an internal review 👍
| * @param {Object} parameters Parameters | ||
| * @param {Object} parameters.tree Dependency tree | ||
| * @param {string} parameters.destPath Target path | ||
| * @param {boolean} [parameters.clearDest=false] Decides whether project should clear the target path before build |
There was a problem hiding this comment.
We discussed this internally and would favor renaming this feature to "cleanDest".
Other tools seem to call this functionality "clean" rather than "clear" as well:
There was a problem hiding this comment.
I agree with @RandomByte, pls rename it to "cleanDest"
| project, | ||
| parentLogger: log | ||
| }).then(async () => { | ||
| if (clearDest) { |
There was a problem hiding this comment.
Also feedback from our internal discussion: It might make sense to move the dest removal part to before the build is started. I.e. Line 273 (feel free to make this function async).
Scenario: If the removal of the dist directory fails because of for example permission issues or because a file is still in use, a user would notice right away. With the current implementation, the user needs to wait for the whole build process to finish before receiving the error and the build is aborted.
There was a problem hiding this comment.
Good point.
Cleaning before the actual build runs means that the target folder is always cleared, even if the new build has failed. But I can live with that.
| * @param {Object} parameters Parameters | ||
| * @param {Object} parameters.tree Dependency tree | ||
| * @param {string} parameters.destPath Target path | ||
| * @param {boolean} [parameters.clearDest=false] Decides whether project should clear the target path before build |
There was a problem hiding this comment.
I agree with @RandomByte, pls rename it to "cleanDest"
| project, | ||
| parentLogger: log | ||
| }).then(async () => { | ||
| if (clearDest) { |
There was a problem hiding this comment.
Good point.
Cleaning before the actual build runs means that the target folder is always cleared, even if the new build has failed. But I can live with that.
|
Replaced by #306 |
This PR address the following issue: #45
Pull Request Checklist