Skip to content
This repository was archived by the owner on Apr 24, 2024. It is now read-only.

feat(cli): add skip-cleanup parameter#251

Open
umutcanbolat wants to merge 2 commits into
Nargonath:masterfrom
umutcanbolat:add-skip-cleanup-param
Open

feat(cli): add skip-cleanup parameter#251
umutcanbolat wants to merge 2 commits into
Nargonath:masterfrom
umutcanbolat:add-skip-cleanup-param

Conversation

@umutcanbolat

Copy link
Copy Markdown

Introduces skip-cleanup param to be able to skip the initial cleanup.
The default value is false.
resolves #250

@umutcanbolat

Copy link
Copy Markdown
Author

I recommend hiding the whitespace changes while reviewing. Otherwise, the change of indentation makes it look like the whole file is edited.
image

@Nargonath
Nargonath changed the base branch from develop to master October 29, 2020 18:27
@Nargonath Nargonath self-assigned this Nov 1, 2020
@Nargonath Nargonath added the enhancement New feature or request label Nov 1, 2020

@Nargonath Nargonath left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your PR needs to be updated since I merged several PR on master. Plus I'm not convinced about migrating to async/await here. I feel it makes the codebase harder to read and lots of the code is synchronous so it doesn't make sense to me to wrap everything in a big async function.

@Nargonath

Copy link
Copy Markdown
Owner

It seems that my other comment on the code didn't go through. I had a question: what were the reasons behind the rewrite to async/await?

That's something I'd be happy to discuss but rather on its own PR and not mixed up with the feature you initially suggested.

@umutcanbolat

Copy link
Copy Markdown
Author

Hi,

I think using async-await here makes it more readable. But we can consider this in another PR as you suggested.

So, with Promise chaining, it would look like this:

Promise.resolve()
  .then(() => skipCleanup || fs.emptyDir(paths.appBuild))
  .then(() => {
    return new Promise((resolve, reject) => {
        // webpack related code here
        return resolve();
    });
  })
  .then(() => copyPublicFolder())
  .then(() => runHook('after initial build hook', spinner, afterInitialBuildHook));

With async-await, could be like this:

skipCleanup || (await fs.emptyDir(paths.appBuild));

await new Promise((resolve, reject) => {
    // webpack related code here
    return resolve();
});

await copyPublicFolder();

runHook('after initial build hook', spinner, afterInitialBuildHook)

@umutcanbolat

Copy link
Copy Markdown
Author

I didn't have time this weekend, but I will resolve conflicts and do the changes during the next week.
Best!

@umutcanbolat

Copy link
Copy Markdown
Author

Hi again. Rebased into master and converted the async-await logic back to the promise chaining as you asked to discuss this in a separate PR.
Have a nice week!

@Nargonath

Copy link
Copy Markdown
Owner

Hi @umutcanbolat! Thanks for updating your PR, I should be able to have a look at it during the week. No worries about the previous week-end. There is no rush. 😉

@umutcanbolat

Copy link
Copy Markdown
Author

Hi! Any update on this?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add parameter to skip initial cleanup

2 participants