Fix recursing to find files in Node <18.17 #332
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `path` and `parentPath` properties of a Dirent object isn't available until 18.17 and 18.20 respectively. To make our package compatible again with older Node versions, I'm switching out `fs.readdir` for `glob`. WIP because I can't figure out how to use the regular async version of `glob`.
So that we will replicate the bug with using fs.readdir together with dirent.paht.
lencioni
reviewed
Feb 18, 2025
| @@ -11,7 +11,7 @@ jobs: | |||
| runs-on: ubuntu-latest | |||
| strategy: | |||
| matrix: | |||
| node: [18, 20, 22] | |||
| node: ['18.16.0', 20, 22] | |||
There was a problem hiding this comment.
Let's update the engines field in package.json to match.
There was a problem hiding this comment.
Oh or maybe with this change we actually end up supporting back more so we can leave engines where it is at now.
lencioni
reviewed
Feb 18, 2025
src/deterministicArchive.js
Outdated
| withFileTypes: true, | ||
| recursive: true, | ||
| const files = Array.from( | ||
| await glob.sync('**/*', { |
There was a problem hiding this comment.
await glob.sync is a bit strange for a couple of reasons:
globSyncis a named export of the glob package, that I think we'd want instead of this if we want synchronous modeawaita sync function is unnecessary- Since we are in an async function can we use the async glob?
There was a problem hiding this comment.
Yeah I kept glob.sync because of issues locally. Turns out I hadn't yarn installed in a while.
`await glob.sync` is a bit strange for a couple of reasons: 1. `globSync` is a named export of the glob package, that I think we'd want instead of this if we want synchronous mode 2. `await` a sync function is unnecessary 3. Since we are in an async function can we use the async glob? This seems to work fine for me, so I think we should make this change.
lencioni
changed the title
lencioni
approved these changes
Feb 18, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
pathandparentPathproperties of a Dirent object isn't available until 18.17 and 18.20 respectively. To make our package compatible again with older Node versions, I'm switching outfs.readdirforglob.