fix(core): throw error if build folder already exists on initial clean by thedevwonder · Pull Request #9112 · facebook/docusaurus

thedevwonder · 2023-06-29T19:09:35Z

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

Fix #9110

Test Plan

local tests

Test links

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

Related issues/PRs

…lean

…-insensitive

netlify · 2023-06-29T19:44:06Z

✅ [V2]

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`d86ac5c`
🔍 Latest deploy log	https://app.netlify.com/sites/docusaurus-2/deploys/649ef212e20d2500082f0b9c
😎 Deploy Preview	https://deploy-preview-9112--docusaurus-2.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

github-actions · 2023-06-29T19:45:02Z

⚡️ Lighthouse report for the deploy preview of this PR

URL	Performance	Accessibility	Best Practices	SEO	PWA	Report
/	🟠 81	🟢 97	🟢 92	🟢 100	🟠 89	Report
/docs/installation	🟠 75	🟢 100	🟢 92	🟢 100	🟠 89	Report

Josh-Cena · 2023-06-30T06:49:34Z

 // More context: https://github.com/facebook/docusaurus/pull/1839

 import path from 'path';
+import * as fs from 'fs';


Use fs-extra and await fs.pathExists(); you can find other examples.

@Josh-Cena I'm pushing a commit that use await pathExists instead of sync func

Josh-Cena · 2023-06-30T08:51:46Z

@thedevwonder Using the sync version is fine here, since we'd better leave the method sync. It shouldn't be too much of a problem.

thedevwonder · 2023-06-30T08:53:41Z

@thedevwonder Using the sync version is fine here, since we'd better leave the method sync. It shouldn't be too much of a problem.

sorry I didn't see the notification and already did it. Should I revert back to sync version?

Josh-Cena · 2023-06-30T08:57:18Z

      return;
    }

+    if (await fs.pathExists(this.outputPath)) {


The outputPath is supposed to exist—this method is used to clean the output. What's problematic is if it's not a directory. You also need to test if it's a file.

Right, I will add a validation for file as well

@Josh-Cena I have a confusion here, pathExistsSync will still return true when build is a file. Then why do we need an additional validation?

There are three cases here:

outputPath doesn't exist -> Always good; removeFiles does nothing

outputPath is a directory -> This is why we have this function; we want to clean its content and prepare for the next build output

outputPath is a file -> This is what Build error The cwd option must be a path to a directory when a file called build exists #9110 is reporting. In this case delSync fails because cwd is a file, not a directory. Logically, we can't clear anything from this file, so we fail.

We don't want to touch the behavior in cases 1 and 2, only give a better error in 3.

I see, I misunderstood earlier. We do not need to change the behaviour when this.outputPath is a directory. So the check that I've put should only validate files and not directories.

@Josh-Cena Reading the original issue, I came across another observation. the error message should occur in case-insensitive filesystems as well. To check if there are no existing files with same name as build but different case, I created a private validator that returns boolean. checks for both files and directories with duplicate name. This will solve:

case where root dir may have diff variations of same file such as BUILD, build, buiLD.

diff variations of same directories.

tested the code in local env.

…ase-insensitive systems

slorber · 2023-06-30T13:35:46Z

+  private checkIfFileNameIsDuplicate(): boolean {
+    const dir = path.dirname(this.outputPath);
+    const baseName = path.basename(this.outputPath).toLowerCase();
+    // eslint-disable-next-line no-restricted-properties
+    const duplicateFiles = fs
+      .readdirSync(dir)
+      .filter((fileName) => fileName.toLowerCase() === baseName);
+    if (duplicateFiles.length > 0) {
+      return true;
+    }
+    return false;
+  }


I think this should be enough

if ( (await fs.pathExists(outputPath)) && (await fs.stat(outputPath)).isFile() ) { throw new Error("..."); }

On:

case insensitive FS: this should throw for files like BUILD or build

case-sensitive FS: if a file named BUILD exists, I think it's allowed to create a folder named build? 🤔

I only have access to a case insensitive file-system. Can someone test my assumption on case sensitive file-system?

@slorber, I think yours is better because it doesn't break the abstraction of whether the FS is case-sensitive or not :)

I think this should be enough

if ( (await fs.pathExists(outputPath)) && (await fs.stat(outputPath)).isFile() ) { throw new Error("..."); }

On:

case insensitive FS: this should throw for files like BUILD or build

case-sensitive FS: if a file named BUILD exists, I think it's allowed to create a folder named build? 🤔

I only have access to a case insensitive file-system. Can someone test my assumption on case sensitive file-system?

I used gitpod.io to test this usecase. It is a case-sensitive system. I changed the code because I thought someone using case-sensitive FS may create a BUILD file and someone not using this FS may find this file throwing an error during the build

What do you think about this, any suggestions? @slorber @Josh-Cena

I think it's fine. Running the same project on different systems can often run into inconsistencies, like some imports that work on case-insensitive systems suddenly fail in case-sensitive ones.

@Josh-Cena I see, keeping it simple then!

@Josh-Cena @slorber I have used the sync version of pathExists and stat functions and fixed the error message. Let me know if this looks good.

I have also tested the code in my local env. Works fine:)

slorber · 2023-06-30T13:36:57Z


+    if (this.checkIfFileNameIsDuplicate()) {
+      throw new Error(
+        `Output directory ${this.outputPath} already exists. Docusaurus needs this directory to save the build output. Either remove the directory or choose a different build directory via '--out-dir'.`,


If it's a directory the role of this plugin is to delete. As @Josh-Cena said the problem is only if a file already exists, so this error message is not good.

@slorber this makes sense, I'll do the change

changes done

slorber

Thanks, LGTM.

Unfortunate that we need to use sync IO but not a big deal.

thedevwonder · 2023-06-30T15:25:54Z

Thanks, LGTM.

Unfortunate that we need to use sync IO but not a big deal.

@slorber is there some way we can check the performance diff in introducing sync I/O functions here? Just curious. Probably build time could be one parameter

thedevwonder · 2023-06-30T15:36:49Z

Thanks, LGTM.
Unfortunate that we need to use sync IO but not a big deal.

@slorber is there some way we can check the performance diff in introducing sync I/O functions here? Just curious. Probably build time could be one parameter

oh I see, build performance is literally one of the checks here, my bad

slorber · 2023-06-30T15:58:10Z

That's fine, this CI fails for other reasons 👍

fix(plugins): throw error if build folder already exists on initial c…

89903b3

…lean

thedevwonder requested review from Josh-Cena, lex111 and slorber as code owners June 29, 2023 19:09

facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 29, 2023

thedevwonder added 3 commits June 29, 2023 19:38

fix: throw an error if the output path name matches the cwd name case…

be98393

…-insensitive

fix: add toLowerCase() to match case

2293e12

fix: add toLowerCase() to match case

6638477

thedevwonder mentioned this pull request Jun 29, 2023

Build error The cwd option must be a path to a directory when a file called build exists #9110

Closed

7 tasks

fix(plugins): add file check using fs

358a8ae