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

Replace clean-webpack-plugin with the core output cleaning feature #1313

Merged
merged 1 commit into from
Sep 2, 2024

Conversation

stof
Copy link
Member

@stof stof commented Aug 30, 2024

Closes #1289

The options configurable by the configurator callback passed as second argument of cleanupOutputBeforeBuild are not the same anymore (the dry option used in the example still exists)

@stof
Copy link
Member Author

stof commented Aug 30, 2024

The ESLint job requires #1311 to pass, because the deprecated valid-jsdoc rule does not support the import() syntax in types.

package.json Outdated Show resolved Hide resolved
Copy link
Member

@Kocal Kocal left a comment

Choose a reason for hiding this comment

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

Thank you for working on this!

package.json Outdated
"css-loader": "^6.7.0",
"css-minimizer-webpack-plugin": "^5.0.0",
"fastest-levenshtein": "^1.0.16",
"mini-css-extract-plugin": "^2.6.0",
"multimatch": "^5",
Copy link
Member

Choose a reason for hiding this comment

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

Can we use picomatch instead?

It has 0 dependencies (where multimatch has 7 dependencies), and weight much less, see graphs:

picomatch is also compatible with CJS modules.

Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

no we cannot (easily). The feature I'm using is precisely the ability to pass multiple patterns including negative patterns (due to the way our cleanupOutputBeforeBuild API looks like), which is the main point of multimatch.

If we decide to break compatibility a lot more by removing support for the first argument of our current cleanupOutputBeforeBuild, I could do it without any dependency.

Copy link
Member Author

Choose a reason for hiding this comment

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

and picomatch might switch to ESM in the future as well (there is a PR opened about that). So just using picomatch won't let us use CJS forever (my expectation is that more and more packages will migrate to ESM)

Copy link
Member Author

Choose a reason for hiding this comment

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

note that this still reduces our dependencies compared to clean-webpack-plugin

Copy link
Member

Choose a reason for hiding this comment

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

What about breaking the cleaning feature and let the user decide what it wants to keep (through output.clean.keep if I remember)?
We could directly pass its configuration to Webpack, it will be easier for us, no need for extra work nor dependencies.
WDYT ?

Copy link
Member Author

@stof stof Aug 30, 2024

Choose a reason for hiding this comment

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

We still need the config to keep the manifest.json for the dev-server (unless the issue mentioned in the code comment is not relevant anymore).

If we can remove that part, I'm fine with doing a bigger BC break dropping the first argument (and letting users configure keep through the callback if needed).

Copy link
Member Author

Choose a reason for hiding this comment

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

However, as I'm not using the dev-server myself (I'm using the watch mode), I don't have a setup to test this.

Copy link
Member

Choose a reason for hiding this comment

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

I can give it a try tomorrow

Copy link
Member

@Kocal Kocal Aug 31, 2024

Choose a reason for hiding this comment

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

Good news, the option output.clean.keep() seems totally ignored in dev-server:

  • Just to be sure, I've removed by public/build folder,
  • Commented cleanedPaths.push('!manifest.json')
  • Ran the dev-server

And file public/build/manifest.json is present, with good URLs, HMR works too, etc...

So we can remove cleanedPaths.push('!manifest.json') and go for a bigger BC :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the PR with the simpler implementation.

@Kocal
Copy link
Member

Kocal commented Sep 2, 2024

Thanks for your modifications!

I've tried your branch locally and everything worked fine, as expected I had to update .cleanupOutputBeforeBuild(['**/*', '!amp/**']) to .cleanupOutputBeforeBuild() (because of argument check, which works fine).

We can merge after a rebase IMO

PS: in the next week, I will try to add E2E tests for the dev-server, to check if it correctly boots, if page is updated after a modification, etc... that will ease this kind of changes.

@stof stof merged commit 7f0347c into symfony:main Sep 2, 2024
28 checks passed
@stof stof deleted the clean branch September 2, 2024 16:31
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.

Remove clean-webpack-plugin
2 participants