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(deps): remove dependency on del #11020

Closed
wants to merge 1 commit into from
Closed

Conversation

benmccann
Copy link

@benmccann benmccann commented Mar 22, 2025

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

del is a very heavy library for what it does with 27 dependencies. del is still included in the repo after this change, but only as a dev dependency

tinyglobby has only 2 dependencies and will end up getting pulled in anyway by copy-webpack-plugin when it's updated to the latest version

Test Plan

using existing tests since no new code was introduced

Test links

Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/

Related issues/PRs

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 22, 2025
Copy link

netlify bot commented Mar 22, 2025

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit 065872d
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/67deed35c5b0810009426986
😎 Deploy Preview https://deploy-preview-11020--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

Report too large to display inline

View full report↗︎

Copy link

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO Report
/ 🟢 90 🟢 98 🟢 100 🟢 100 Report
/docs/installation 🔴 48 🟢 97 🟢 100 🟢 100 Report
/docs/category/getting-started 🟠 72 🟢 100 🟢 100 🟠 86 Report
/blog 🟠 63 🟢 96 🟢 100 🟠 86 Report
/blog/preparing-your-site-for-docusaurus-v3 🟠 64 🟢 92 🟢 100 🟢 100 Report
/blog/tags/release 🟠 61 🟢 96 🟢 100 🟠 86 Report
/blog/tags 🟠 72 🟢 100 🟢 100 🟠 86 Report

@slorber
Copy link
Collaborator

slorber commented Mar 24, 2025

Thanks for opening the discussion. I agree we can do some cleanup here.

del is a very heavy library for what it does with 27 dependencies. del is still included in the repo after this change, but only as a dev dependency

From what I see, it's 8, not 27.

And many of these transitive dependencies (like globby or p-map) are used in other places

https://www.npmjs.com/package/del/v/6.1.1?activeTab=dependencies

tinyglobby has only 2 dependencies

We already use globby in other places, so I'd prefer not include a 2nd globbing tool

In the future, we'll probably remove globby in favor of Node built-in globbing tool: https://nodejs.org/api/fs.html#fsglobpattern-options-callback


and will end up getting pulled in anyway by copy-webpack-plugin when it's updated to the latest version

Not sure what you mean here. We copied that code from a legacy unmaintained lib that we historically use, and don't plan to "upgrade" or revert the forking: https://github.com/johnagan/clean-webpack-plugin

It would actually be more useful to create our own CleanWebpackPlugin, removing all the things that we don't need from the original, making it async, keeping it minimal, dependency free.

        // Remove/clean build folders before building bundles.
        new CleanWebpackPlugin({verbose: false}),

This is historical code, but I wonder if we really need a plugin and libs to delete the website/build folder 🤷‍♂️

I didn't study in depth what this plugin does exactly, but I'd probably simply replace that whole plugin with await rimraf(buildOutput) and decouple this logic from the bundler plugin system.

That's even what the historical plugin recommends 😆

  /**
   * Initially remove files from output directory prior to build.
   *
   * Only happens once.
   *
   * Warning: It is recommended to initially clean your build directory outside
   * of webpack to minimize unexpected behavior.
   */
  handleInitial(): void {

@benmccann
Copy link
Author

Regarding the 27 dependencies, I was including transitive dependencies as well: http://npmgraph.js.org/?q=del

I think swapping out globby for tinyglobby would be a good idea as well. I'd be happy to take a stab at that if you're amenable. I agree using the version supplied by Node is best in the long-term, but since it's only available in Node 22, I think it will be at least a couple more years until it can be used.

I wasn't referring clean-webpack-plugin, but to copy-webpack-plugin used here:

"copy-webpack-plugin": "^11.0.0",

@slorber
Copy link
Collaborator

slorber commented Mar 24, 2025

Ah I see thanks

I'd prefer to stick to globby for v3, but we can consider changing the dependency for v4 globally if it's safe and doesn't cause weird side effects

@benmccann
Copy link
Author

Sure. Would it be better to switch del to tinyglobby, switch globby to tinyglobby, and upgrade copy-webpack-plugin in a single PR or in separate PRs?

@slorber
Copy link
Collaborator

slorber commented Mar 26, 2025

Sure. Would it be better to switch del to tinyglobby, switch globby to tinyglobby, and upgrade copy-webpack-plugin in a single PR or in separate PRs?

I'm not sure what you mean.

As explained above, the thing I'm willing to merge for v3 is to remove the copy-webpack-plugin entirely.

You can already work on replacing globby globally with tinyglobby as a second PR, but I will only merge it later once we are ready to start merging breaking changes for v4. Globby is implicitly part of our public API surface, and I'll only consider merging it for v3 only if you can convince me it's a 100% safe retro-compatible replacement.

@benmccann
Copy link
Author

I'm not sure what you mean.

I meant for v4. I'd be happy to put together a PR now to have it queued up, but didn't know if you'd prefer a single PR or separate PRs for the work to be done

the thing I'm willing to merge for v3 is to remove the copy-webpack-plugin entirely

I'm quite confused as to whether copy-webpack-plugin or clean-webpack-plugin was meant as both have been referred to

Globby is implicitly part of our public API surface, and I'll only consider merging it for v3 only if you can convince me it's a 100% safe retro-compatible replacement.

tinyglobby works in a compatible manner, but has a smaller set of options than globby. Most projects don't need the extra options that globby offers. If you expose all of tinyglobby's options to users then you wouldn't be able to migrate without a breaking change, but if the options are only used internally then users shouldn't notice a difference

@slorber
Copy link
Collaborator

slorber commented Mar 27, 2025

I'm quite confused as to whether copy-webpack-plugin or clean-webpack-plugin was meant as both have been referred to

Sorry, I meant we should remove clean-webpack-plugin entirely.


What I propose is that I'll study if clean-webpack-plugin can be safely removed, if I don't miss anything important about its role in Docusaurus, and eventually remove it. I'd like to also know how badly those sync calls affected performance.


At the same time you can work on replacing globby with tinyglobby, and we'll see if the options are good enough and if this triggers and CI regression or roadblock. No need to handle clean-webpack-plugin for now because I'm likely to remove it today anyway (if I can, otherwise I'll let you know).

Globby is part of our public API surface because we pass user/plugin inputs to it, but we are using it with a limited number of options, so let's see if the tinyglobby options are good enough, and more importantly, are they retrocompatible? I Don't see anywhere tinyglobby mentioning retro-compatibility with globby as a goal (even with removed options), so I'm afraid of possible side effects. If both projects shared the same test suite, that would be reassuring for me to consider it for v3, otherwise it can wait for v4.

@slorber
Copy link
Collaborator

slorber commented Mar 27, 2025

Superseded by #11037

From my investigation, we can safely remove the clean-webpack-plugin, and this has various benefits:

  • remove legacy forked dependency code
  • remove del dependency
  • improve prod bundler perf by removing synchronous delete calls, causing at least 150ms bundling delay my M3 and a small site

Thanks for opening the discussion, and happy to get rid of it.

I'm going to close this PR as del is now gone, but I'm open to other PRs modernizing our infra such as replacing globby by tinyglobby 👍

@slorber slorber closed this Mar 27, 2025
@benmccann
Copy link
Author

Thank you!!

I took a look at replacing globby with tinyglobby. The main blocker I see is that tinyglobby only supports a boolean for the expandDirectories option and docusaurus passes a string[] for that option. I've filed an issue in the tinyglobby repo requesting that support: SuperchupuDev/tinyglobby#103

@slorber
Copy link
Collaborator

slorber commented Mar 28, 2025

I see thanks

So what I understand is the our docusaurus write-heading-ids CLI might be the only incompatible place:

export async function writeHeadingIds(
  siteDirParam: string = '.',
  files: string[] = [],
  options: WriteHeadingIDOptions = {},
): Promise<void> {
  const siteDir = await fs.realpath(siteDirParam);

  const markdownFiles = await safeGlobby(
    files ?? (await getPathsToWatch(siteDir)),
    {
      expandDirectories: ['**/*.{md,mdx}'],
    },
  );

  // ...
}

Using this lib under the hood: https://github.com/kevva/dir-glob


To be honest I think we can just replace that with expandDirectories: true and filter the output manually, only keeping md/mdx files.

I'm not sure it's as efficient, but this is not really the most perf critical part of Docusaurus so it should be fine.

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

Successfully merging this pull request may close these issues.

3 participants