Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

lib: avoid excluding symlinks in recursive fs.readdir with filetypes #55714

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

juanarbol
Copy link
Member

Fixes: #52663

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Nov 4, 2024
@juanarbol juanarbol added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 4, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 4, 2024
@nodejs-github-bot
Copy link
Collaborator

Copy link

codecov bot commented Nov 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.42%. Comparing base (6af5c4e) to head (a8a6aa0).
Report is 827 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55714      +/-   ##
==========================================
+ Coverage   88.41%   88.42%   +0.01%     
==========================================
  Files         654      654              
  Lines      187757   187863     +106     
  Branches    36129    36148      +19     
==========================================
+ Hits       166003   166119     +116     
+ Misses      14997    14982      -15     
- Partials     6757     6762       +5     
Files with missing lines Coverage Δ
lib/fs.js 98.26% <100.00%> (+0.11%) ⬆️

... and 54 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Ethan-Arrowood
Copy link
Contributor

Please add a test; otherwise lgtm

@Ethan-Arrowood Ethan-Arrowood self-assigned this Nov 6, 2024
@juanarbol
Copy link
Member Author

Please add a test; otherwise lgtm

Will do

@juanarbol juanarbol force-pushed the juan/fs-readdir branch 2 times, most recently from 0330a1d to 10a71b9 Compare November 14, 2024 01:36
@juanarbol
Copy link
Member Author

@Ethan-Arrowood there we go!

Fixes: nodejs#52663
Signed-off-by: Juan José Arboleda <soyjuanarbol@gmail.com>
@juanarbol
Copy link
Member Author

@Ethan-Arrowood you were right, I've addressed your suggestions and tested the thing w/ main branch build.

@nodejs-github-bot
Copy link
Collaborator

@juanarbol
Copy link
Member Author

ping @Ethan-Arrowood

Copy link
Contributor

@Ethan-Arrowood Ethan-Arrowood left a comment

Choose a reason for hiding this comment

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

lgtm!

@Ethan-Arrowood Ethan-Arrowood added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 21, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 21, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@juanarbol juanarbol added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 24, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 24, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 24, 2024

@juanarbol juanarbol added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 25, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 25, 2024
aduh95 pushed a commit that referenced this pull request Dec 13, 2024
Fixes: #52663
Signed-off-by: Juan José Arboleda <soyjuanarbol@gmail.com>
PR-URL: #55714
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 pushed a commit that referenced this pull request Dec 13, 2024
Fixes: #52663
Signed-off-by: Juan José Arboleda <soyjuanarbol@gmail.com>
PR-URL: #55714
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Reviewed-By: James M Snell <jasnell@gmail.com>
aduh95 pushed a commit that referenced this pull request Dec 13, 2024
Fixes: #52663
Signed-off-by: Juan José Arboleda <soyjuanarbol@gmail.com>
PR-URL: #55714
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Reviewed-By: James M Snell <jasnell@gmail.com>
ruyadorno pushed a commit that referenced this pull request Jan 5, 2025
Fixes: #52663
Signed-off-by: Juan José Arboleda <soyjuanarbol@gmail.com>
PR-URL: #55714
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere added a commit to electron/electron that referenced this pull request Jan 28, 2025
nodejs/node#55714

This doesn't currently play well with ASAR - this should be fixed in a follow up
codebytere added a commit to electron/electron that referenced this pull request Jan 28, 2025
nodejs/node#55714

This doesn't currently play well with ASAR - this should be fixed in a follow up
codebytere added a commit to electron/electron that referenced this pull request Jan 28, 2025
nodejs/node#55714

This doesn't currently play well with ASAR - this should be fixed in a follow up
jkleinsc pushed a commit to electron/electron that referenced this pull request Jan 29, 2025
* chore: bump node in DEPS to v22.13.1

* chore: fixup GN build file

* nodejs/node#55529
* nodejs/node#55798
* nodejs/node#55530

* module: simplify --inspect-brk handling

nodejs/node#55679

* src: fix outdated js2c.cc references

nodejs/node#56133

* crypto: include openssl/rand.h explicitly

nodejs/node#55425

* build: use variable for crypto dep path

nodejs/node#55928

* crypto: fix RSA_PKCS1_PADDING error message

nodejs/node#55629

* build: use variable for simdutf path

nodejs/node#56196

* test,crypto: make crypto tests work with BoringSSL

nodejs/node#55491

* fix: suppress clang -Wdeprecated-declarations in libuv

libuv/libuv#4486

* deps: update libuv to 1.49.1

nodejs/node#55114

* test: make test-node-output-v8-warning more flexible

nodejs/node#55401

* [v22.x] Revert "v8: enable maglev on supported architectures"

nodejs/node#54384

* fix: potential WIN32_LEAN_AND_MEAN redefinition

c-ares/c-ares#869

* deps: update nghttp2 to 1.64.0

nodejs/node#55559

* src: provide workaround for container-overflow

nodejs/node#55591

* build: use variable for simdutf path

nodejs/node#56196

* chore: fixup patch indices

* fixup! module: simplify --inspect-brk handling

* lib: fix fs.readdir recursive async

nodejs/node#56041

* lib: avoid excluding symlinks in recursive fs.readdir with filetypes

nodejs/node#55714

This doesn't currently play well with ASAR - this should be fixed in a follow up

* test: disable CJS permission test for config.main

This has diverged as a result of our revert of
src,lb: reducing C++ calls of esm legacy main resolve

* fixup! lib: fix fs.readdir recursive async

* deps: update libuv to 1.49.1

nodejs/node#55114

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
codebytere added a commit to electron/electron that referenced this pull request Feb 3, 2025
nodejs/node#55714

This doesn't currently play well with ASAR - this should be fixed in a follow up
codebytere added a commit to electron/electron that referenced this pull request Feb 10, 2025
nodejs/node#55714

This doesn't currently play well with ASAR - this should be fixed in a follow up
codebytere added a commit to electron/electron that referenced this pull request Feb 10, 2025
nodejs/node#55714

This doesn't currently play well with ASAR - this should be fixed in a follow up
codebytere added a commit to electron/electron that referenced this pull request Feb 14, 2025
nodejs/node#55714

This doesn't currently play well with ASAR - this should be fixed in a follow up
codebytere added a commit to electron/electron that referenced this pull request Feb 14, 2025
* chore: bump node in DEPS to v22.13.0

* chore: bump node in DEPS to v22.13.1

* src: move evp stuff to ncrypto

nodejs/node#54911

* crypto: add Date fields for validTo and validFrom

nodejs/node#54159

* module: fix discrepancy between .ts and .js

nodejs/node#54461

* esm: do not interpret "main" as a URL

nodejs/node#55003

* src: modernize likely/unlikely hints

nodejs/node#55155

* chore: update patch indices

* crypto: add validFromDate and validToDate fields to X509Certificate

nodejs/node#54159

* chore: fixup perfetto patch

* fix: clang warning in simdjson

* src: add receiver to fast api callback methods

nodejs/node#54408

* chore: fixup revert patch

* fixup! esm: do not interpret "main" as a URL

* fixup! crypto: add Date fields for validTo and validFrom

* fix: move ArrayBuffer test patch

* src: fixup Error.stackTraceLimit during snapshot building

nodejs/node#55121

* fix: bad rebase

* chore: fixup amaro

* chore: address feedback from review

* src: revert filesystem::path changes

nodejs/node#55015

* chore: fixup GN build file

* nodejs/node#55529
* nodejs/node#55798
* nodejs/node#55530

* module: simplify --inspect-brk handling

nodejs/node#55679

* src: fix outdated js2c.cc references

nodejs/node#56133

* crypto: include openssl/rand.h explicitly

nodejs/node#55425

* build: use variable for crypto dep path

nodejs/node#55928

* crypto: fix RSA_PKCS1_PADDING error message

nodejs/node#55629

* build: use variable for simdutf path

nodejs/node#56196

* test,crypto: make crypto tests work with BoringSSL

nodejs/node#55491

* fix: suppress clang -Wdeprecated-declarations in libuv

libuv/libuv#4486

* deps: update libuv to 1.49.1

nodejs/node#55114

* test: make test-node-output-v8-warning more flexible

nodejs/node#55401

* [v22.x] Revert "v8: enable maglev on supported architectures"

nodejs/node#54384

* fix: potential WIN32_LEAN_AND_MEAN redefinition

c-ares/c-ares#869

* deps: update nghttp2 to 1.64.0

nodejs/node#55559

* src: provide workaround for container-overflow

nodejs/node#55591

* build: use variable for simdutf path

nodejs/node#56196

* chore: fixup patch indices

* fixup! module: simplify --inspect-brk handling

* lib: fix fs.readdir recursive async

nodejs/node#56041

* lib: avoid excluding symlinks in recursive fs.readdir with filetypes

nodejs/node#55714

This doesn't currently play well with ASAR - this should be fixed in a follow up

* test: disable CJS permission test for config.main

This has diverged as a result of our revert of
src,lb: reducing C++ calls of esm legacy main resolve

* fixup! lib: fix fs.readdir recursive async

* deps: update libuv to 1.49.1

nodejs/node#55114

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
@marco-ippolito marco-ippolito added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Mar 6, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

withFileTypes has different behavior in fs.readdir when reading symbolic directories
5 participants