-
Notifications
You must be signed in to change notification settings - Fork 103
feat(headers-parser): replace dependencies with native APIs #7147
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
Open
43081j
wants to merge
1
commit into
netlify:main
Choose a base branch
from
43081j:jg/the-purge-headers-parser
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: netlify/build
Length of output: 340
🏁 Script executed:
Repository: netlify/build
Length of output: 15650
🏁 Script executed:
Repository: netlify/build
Length of output: 6586
🌐 Web query:
Node.js fs.access default flags F_OK read permissions R_OK file permission error documentation💡 Result:
In Node.js, the fs.access function is used to test a user's permissions for a file or directory [1][2]. The mode argument is an optional integer that specifies the accessibility checks to be performed, with a default value of fs.constants.F_OK [3][4]. Key details regarding fs.access constants and usage: Default Behavior: The default value for the mode argument is fs.constants.F_OK, which indicates that the file is visible to the calling process (i.e., it exists) [3][4]. Permission Constants: The mode argument can also be a bitwise OR of any of the following constants: fs.constants.F_OK: Visible to the calling process [3][4]. fs.constants.R_OK: Readable by the calling process [1][5]. fs.constants.W_OK: Writable by the calling process [1][5]. fs.constants.X_OK: Executable by the calling process [1][5]. Error Handling: If the accessibility check fails, the function returns a rejected promise (when using fsPromises.access) or invokes the callback with an error object (when using fs.access) [1][2]. Important Recommendation: Node.js documentation explicitly advises against using fs.access to check for accessibility before performing operations like fs.open, fs.readFile, or fs.writeFile [1][2]. Doing so creates a race condition, as the file's state could change between the access check and the actual operation [1][6]. Instead, it is best practice to attempt the operation directly and handle any errors that occur [1][4]. Additionally, users should be aware that on Windows, fs.access does not check Access Control Lists (ACLs), so it may return success even if ACLs would otherwise restrict the user from reading or writing to the file [7][8].
Citations:
Replace the
fs.accesspreflight with direct-read error handling.These checks default to
F_OK, so they don’t reliably detect unreadable files and still race with the laterfs.readFile/TOML parse calls. Remove the preflight and let the actual read/parse path handle the intended empty-result behavior consistently.📍 Affects 2 files
packages/headers-parser/src/line_parser.ts#L24-L26(this comment)packages/headers-parser/src/netlify_config_parser.ts#L12-L16🤖 Prompt for AI Agents