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

fix(docs): asset cleaning errors on windows #2668

Merged
merged 1 commit into from
Mar 20, 2023
Merged

fix(docs): asset cleaning errors on windows #2668

merged 1 commit into from
Mar 20, 2023

Conversation

tsa96
Copy link
Contributor

@tsa96 tsa96 commented Mar 19, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

rimraf v4 recently removed support for glob patterns, then subsequently added them back, but requiring the { glob: true } in the options passed to rimraf API methods. My understanding is, on POSIX-compliant OSs, it'll use the native shell handling of glob patterns anyway, but on Windows, now needs to passed this option for it to use the node-glob, which handles glob patterns on Windows.

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Docs
  • Other... Please describe:

What is the current behavior?

As mentioned in the below issue, the rimraf.sync call in cleanGeneratedFiles.ts will error with "Illegal characters in path." when the ${OUTPUT_PATH}/{docs,*.json} glob is passed in, apparently only on Windows (I'm on Windows 11 and get the exact same output as #2635).

Issue Number: #2635

What is the new behavior?

Explicitly uses the built-in glob handling, which now runs fine on Windows. This may change behavior on other OSs, which at least since rimraf v4 were doing glob handling using native shell functionality, but I assume that getting identical behavior between different operating systems is why you're using rimraf in the first place.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Copy link
Member

@micalevisk micalevisk left a comment

Choose a reason for hiding this comment

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

I tested npm run build:watch on Ubuntu.

thanks!

@kamilmysliwiec kamilmysliwiec merged commit 5375f4a into nestjs:master Mar 20, 2023
@kamilmysliwiec
Copy link
Member

lgtm

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