Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: auto clean dist folder before build #369

Merged
merged 5 commits into from
Jan 13, 2024
Merged

feat: auto clean dist folder before build #369

merged 5 commits into from
Jan 13, 2024

Conversation

nnecec
Copy link
Contributor

@nnecec nnecec commented Jan 8, 2024

Implement the feature #358 (comment). Here are changes:

  1. add cleanBuildOutputDir before runBundle
  2. add cleanDir in utils.ts
  3. fix compile.test.ts. Because output dir will be clean at each bundle, we need run tests after finish bundle immediately.
  4. When I run git commit , here is a hint hint: The '.husky/pre-commit' hook was ignored because it's not set as executable. I found a solution hook was ignored because it's not set as executable typicode/husky#1177. After I executed chmod ug+x .husky/*, the pre-commit file has been update.

}

// build minified file
await bundle(inputFileName, {
Copy link
Owner

Choose a reason for hiding this comment

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

Can we keep this as its before? I want two bundle() calls finished then compare so the output are all there to check instead of bailing first

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will clean the dist directory when executing the bundle function. So, if we keep this as before, I think we need to provide a new option in BundleConfig, or even support a new cli arg like --clean or --noClean, so that we can specify whether the dist needs to be deleted when the bundle is executed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have any suggestions?

Copy link
Owner

Choose a reason for hiding this comment

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

Good call, we can clean by default, and add one more--no-clean arg for not doing it. Also need a test for that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just add a --no-clean option and add test to integration tests.

Btw, I am a little confused, I can't figure out what difference between cli.test and integration.test. I feel like both are integration test cases.

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Maybe I could answer that for you, cli tests focus on targeting the CLI itself and the --args whereas integration is more of an E2E test that runs to meet the specific use cases.

Copy link
Owner

Choose a reason for hiding this comment

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

yeah would be good if we can move to cli test later, but it's not blocking for merging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I could answer that for you, cli tests focus on targeting the CLI itself and the --args whereas integration is more of an E2E test that runs to meet the specific use cases.

thanks!

src/bundle.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
Copy link
Owner

@huozhi huozhi left a comment

Choose a reason for hiding this comment

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

Thanks!

Would be great if you use other branch or change the fork repo setting next time, I was trying to modify the code but seems not able to do it. Will follow up in different PRs

! [remote rejected] clean -> clean (permission denied)
error: failed to push some refs to 'https://github.com/nnecec/bunchee'

@huozhi huozhi merged commit c7fcba9 into huozhi:main Jan 13, 2024
3 checks passed
@nnecec
Copy link
Contributor Author

nnecec commented Jan 13, 2024

Thanks!

Would be great if you use other branch or change the fork repo setting next time, I was trying to modify the code but seems not able to do it. Will follow up in different PRs

! [remote rejected] clean -> clean (permission denied)
error: failed to push some refs to 'nnecec/bunchee'

Got it! I'm sorry I didn't know that.

@huozhi
Copy link
Owner

huozhi commented Jan 13, 2024

No worries, probably just some random GitHub limitations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants