X Tutup
The Wayback Machine - https://web.archive.org/web/20220228001356/https://github.com/webpack/webpack/pull/11561
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

Jump to bottom

(feat): add output.clean #11561

Merged
merged 18 commits into from Feb 2, 2021
Merged

(feat): add output.clean #11561

merged 18 commits into from Feb 2, 2021

Conversation

@smelukov
Copy link
Member

@smelukov smelukov commented Sep 30, 2020 •
edited

What kind of change does this PR introduce?

feature
closes https://github.com/webpack/webpack/projects/5#card-40756723

usage

{
  output: {
    clean: true
  }
}

or

{
  output: {
    clean: {
      enabled: true
    }
  }
}

dry-run

Not to delete the assets, just console.log about

ignore

by property

output: {
  clean: {
    ignore: /\/ignored\/dir\//
  }
},

or

output: {
  clean: {
    ignore(asset) {
      return asset.includes("ignored/dir");
    }
  }
},

by hook

webpack.CleanPlugin.getCompilationHooks(compilation).ignore.tap(
  "Test",
  ignore => {
    ignore(/\/ignored\/dir\//);
    // or
    ignore(asset => asset.includes("ignored/too"));
  }
);

Did you add tests for your changes?

yes

Does this PR introduce a breaking change?

no

What needs to be documented once your changes are merged?

about output.clean
@webpack-bot
Copy link
Contributor

@webpack-bot webpack-bot commented Sep 30, 2020 •
edited by smelukov

For maintainers only:

  • This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
  • This needs to be backported to webpack 4 (issue will be created when merged)

@smelukov smelukov requested a review from sokra Oct 1, 2020
alexander-akait
alexander-akait reviewed Oct 2, 2020
View changes
Copy link
Member

@alexander-akait alexander-akait left a comment

Can we add hooks? Why?

Some plugins may copy assets without using compilation.assets, so we could add additional files to delete, so we don't need extra option(s) to achieve behavior as clean-webpack-plugin

@smelukov
Copy link
Member Author

@smelukov smelukov commented Oct 2, 2020

Some plugins may copy assets without using compilation.assets

As @sokra said before

We take them into account as long they are using the compilation.assets mechanism. Many do, but not all. But it should be a goal that they use it, as other plugins might use the information in compilation.assets for other things, e. g. service worker or reporting
During migration one might not be able to use clean: true yet, but that's fine as there are alternatives that can be used. Core features should push towards best practices...

...

I think most plugins write there files in the emit hook. When we do the initial cleanup before that hook that should not break these plugins.

@alexander-akait
Copy link
Member

@alexander-akait alexander-akait commented Oct 2, 2020

Yep, I known, just idea how we can solve this without problems, sometimes you real need to copy files in dist directory without extra plugins, i.e. manifests/config files for env/etc, adding hook(s) should not be hard and provide flexibility solution

@smelukov
Copy link
Member Author

@smelukov smelukov commented Oct 2, 2020

I may add ignore regexp

@smelukov
Copy link
Member Author

@smelukov smelukov commented Oct 2, 2020 •
edited

something like

clean: {
  ignore: /^\/static-files\//
}

@alexander-akait
Copy link
Member

@alexander-akait alexander-akait commented Oct 2, 2020

@smelukov Maybe this will be a good solution too, but most often plugins know what they are adding, which means they can form ignored files on their side (why I think hook(s) is good idea)

@smelukov
Copy link
Member Author

@smelukov smelukov commented Oct 2, 2020

@evilebottnawi Ok, I think this is a good idea
I will add regexp and a hook to solve the different cases

@smelukov
Copy link
Member Author

@smelukov smelukov commented Oct 2, 2020

@evilebottnawi look at the PR description to see some updates ;)

@webpack-bot
Copy link
Contributor

@webpack-bot webpack-bot commented Oct 2, 2020

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

sokra
sokra requested changes Oct 2, 2020
View changes
Copy link
Member

@sokra sokra left a comment

Reviewed that a few commits ago but forgot to submit. Hopefully it's still up to date

schemas/WebpackOptions.json Outdated
"type": "boolean"
},
{
"tsType": "(import('../declarations/plugins/CleanPlugin').CleanPluginArgument)"
Copy link
Member

@sokra sokra Oct 2, 2020

Choose a reason for hiding this comment

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

Missing schema. Type info only doesn't work for validation

schemas/WebpackOptions.json Outdated
@@ -2037,6 +2048,10 @@
"chunkLoadingGlobal": {
"$ref": "#/definitions/ChunkLoadingGlobal"
},
"clean": {
"description": "Remove non-webpack assets from the output directory.",
Copy link
Member

@sokra sokra Oct 2, 2020

Choose a reason for hiding this comment

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

The description is a bit unclear to me

lib/CleanPlugin.js
directories: collectedDirectories
};
}

Copy link
Member

@sokra sokra Oct 2, 2020

Choose a reason for hiding this comment

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

Whole plugin should use async fs methods.

On incremental build it should not read from filesystem, but assume old state

Copy link
Member Author

@smelukov smelukov Oct 3, 2020

Choose a reason for hiding this comment

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

On incremental build it should not read from filesystem, but assume old state

There is emittedOnce-flag for that

Copy link
Member

@sokra sokra Oct 5, 2020

Choose a reason for hiding this comment

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

There is emittedOnce-flag for that

ah I see. The flag causes the plugin to do nothing on incremental compilation, but instead it should e.g. remove assets from the first compilation that are no longer part of the second compilation.

I would solve that by storing the filesystem state after the first compilation (which directories now contain which files) and use that instead of reading the filesystem in the second compilation. As said we assume that nothing expect webpack changes the output directory while webpack is running. This should be pretty efficient, no fs calls should be done expect for removing old assets, when they are no longer referenced.

lib/CleanPlugin.js Outdated
}, "");
}

const collected = readDir(compiler.outputFileSystem, outputPath);
Copy link
Member

@sokra sokra Oct 2, 2020

Choose a reason for hiding this comment

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

It's unnecessary to read the whole fs structure. It should be enough to read only the assetDirectories to know what to do.

Copy link
Member Author

@smelukov smelukov Oct 3, 2020 •
edited

Choose a reason for hiding this comment

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

It's unnecessary to read the whole fs structure

Then how we know about files and directories that not a webpack-assets? assetsDirectories contains directories only from compilation.assets

Copy link
Member

@sokra sokra Oct 5, 2020

Choose a reason for hiding this comment

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

assetsDirectories contains directories only from compilation.assets

Yep that is perfect. You only need to read these directories to have full information what need to be done.
Read any other directory that's not in assetsDirectories would be wasted as it would be completely deleted anyway as the folder is extraneous in some directory of assetsDirectories anyway.

lib/CleanPlugin.js Outdated

if (
!compiler.outputFileSystem ||
!compiler.outputFileSystem.existsSync(outputPath)
Copy link
Member

@sokra sokra Oct 2, 2020

Choose a reason for hiding this comment

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

Unnecessary, we can check for ENOENT error when reading.

lib/CleanPlugin.js Outdated
const assetsDirectories = new Set();

for (const asset in compilation.assets) {
const parts = asset.split(path.sep).slice(0, -1);
Copy link
Member

@sokra sokra Oct 2, 2020

Choose a reason for hiding this comment

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

Asset filename need to be normalized. Asset filename might be outside of the output directory.

Copy link
Member Author

@smelukov smelukov Oct 3, 2020 •
edited

Choose a reason for hiding this comment

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

are you about path.resolve?

Copy link
Member

@sokra sokra Oct 5, 2020

Choose a reason for hiding this comment

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

Usually asset contains always / as it's considered as filename (it's not an absolute path), but we accept \ too just in case. So at least you need to split(/[\\\/]+/), remove any . and handle any ... As this could get tricky it might make sense to path.resolve the asset in the output.path and work with absolute paths only.

alexander-akait
alexander-akait reviewed Oct 7, 2020
View changes
lib/CleanPlugin.js Outdated Show resolved Hide resolved
alexander-akait
alexander-akait reviewed Oct 7, 2020
View changes
lib/CleanPlugin.js Outdated Show resolved Hide resolved
alexander-akait
alexander-akait reviewed Oct 7, 2020
View changes
lib/CleanPlugin.js Outdated Show resolved Hide resolved
alexander-akait
alexander-akait reviewed Oct 7, 2020
View changes
lib/CleanPlugin.js
this.logger = compiler.getInfrastructureLogger("webpack.Clean");
this.fs = compiler.outputFileSystem;

compiler.hooks.emit.tapAsync(
Copy link
Member

@alexander-akait alexander-akait Oct 7, 2020

Choose a reason for hiding this comment

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

Not sure it is safe to use emit, because I see a lot of plugins with stage: 99999/any number and using emit, maybe better introduce new hook, but let's wait @sokra answer

Copy link
Member Author

@smelukov smelukov Oct 7, 2020

Choose a reason for hiding this comment

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

I think that a new hook will not solve this problem, because any 3-rd party plugin may use this new hook too )

Copy link
Member

@alexander-akait alexander-akait Oct 7, 2020

Choose a reason for hiding this comment

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

We can implement hook cleanEmittedAssets, so nobody will use it for emitting 😄

Copy link
Member Author

@smelukov smelukov Oct 7, 2020

Choose a reason for hiding this comment

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

But they can )))

@smelukov smelukov closed this Oct 7, 2020
@smelukov smelukov reopened this Oct 7, 2020
@smelukov smelukov requested a review from sokra Oct 9, 2020
@sokra sokra mentioned this pull request Dec 17, 2020
Closed
@martinbroos
Copy link

@martinbroos martinbroos commented Feb 1, 2021

Is this feature gonna be released for webpack 5 soon? I'm facing problems with the Clean Webpack Plugin and typescript in webpack 5. This option would make this plugin no longer required.

Thanks for working on this!

@smelukov
Copy link
Member Author

@smelukov smelukov commented Feb 2, 2021

@sokra can you review, please?

@sokra sokra merged commit 36306a0 into webpack:master Feb 2, 2021
13 checks passed
@sokra
Copy link
Member

@sokra sokra commented Feb 2, 2021

Thanks

@webpack-bot webpack-bot mentioned this pull request Feb 2, 2021
Closed
@webpack-bot
Copy link
Contributor

@webpack-bot webpack-bot commented Feb 2, 2021

I've created an issue to document this in webpack/webpack.js.org.

yvettep321
yvettep321 reviewed Feb 5, 2021
View changes
Copy link

@yvettep321 yvettep321 left a comment

_thank you _

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

@alexander-akait alexander-akait

@yvettep321 yvettep321

@sokra sokra

Assignees
No one assigned
Labels
PR: CI-ok PR: review-outdated
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet
6 participants
@smelukov @webpack-bot @alexander-akait @martinbroos @sokra @yvettep321
X Tutup