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

chore(cli): add --clean option to build command #87

Closed
wants to merge 0 commits into from

Conversation

shulaoda
Copy link

Related Links

Related to #46

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

.description('build the library for production')
.action(async (options: BuildOptions) => {
try {
const rslibConfig = await loadConfig(options.config, options.envMode);

if (options.clean) {
const distDir = rslibConfig.output?.distPath?.root ?? OUTPUT_DIST_DIR;
Copy link
Collaborator

@Timeless0911 Timeless0911 Aug 14, 2024

Choose a reason for hiding this comment

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

It's raw Rslib config, we should get distDir from final Rsbuild environment config.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Rsbuild already support clean output.cleanDistPath, may be you should implemeny clean feature in plugin-dts, to add an option of clean, and in default rslib core logic, when we use builtin plugin-dts, pass clean: true to it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The issue was written early and may have some problems now.

Copy link
Author

Choose a reason for hiding this comment

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

Would it be confusing to include the clean option in plugin-dts?

Copy link
Author

Choose a reason for hiding this comment

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

Because clean is a global build option, not plugin-dts.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Js output will be cleaned according to Rsbuild output.cleanDistPath by default, the DTS output should align with this behaviour. Maybe we don't need a cli option to do that, just in built-in logic.

Copy link
Author

Choose a reason for hiding this comment

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

Maybe we don't need a cli option to do that, just in built-in logic.

Agree

Copy link
Author

Choose a reason for hiding this comment

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

However, Rsbuild seems to default to clear dist. What is the significance of doing so? Does plugin-dts not support default clear?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The dist maybe dirty in local development, clean behavihour can ensure the new output will not be influenced by existed output.

Copy link
Collaborator

Choose a reason for hiding this comment

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

plugin-dts have not supported it yet.

We should clean:

  • bundleless mode: the tsc emit path
  • bundle mode: the tsc emit temp path .rslib/declarations and api-extractor emit path

@Timeless0911
Copy link
Collaborator

I have changed the feature issue
image

What do you think about the clean feature?

@shulaoda shulaoda marked this pull request as draft August 14, 2024 04:00
@shulaoda shulaoda closed this Aug 20, 2024
@shulaoda
Copy link
Author

I tried to rewrite, but it seems that an empty commit caused PR to close.

@fi3ework
Copy link
Member

Feel free to re-create one and link to this PR.

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.

3 participants