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

lib: fix handling of non-object options parameter in fs/watchFile #54157

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kimtaejin3
Copy link
Contributor

Problem I resolved

const { watchFile } = require("fs");

// watchFile(filename:string|Buffer|URL, options:object, listener:function)
watchFile("./note.txt", "string", (curr, prev) => {});

It causes TypeError:

TypeError [ERR_INVALID_ARG_TYPE]: The "listener" argument must be of type function. Received type string ('string')
    at watchFile (node:fs:2514:3)
   ...
}

I think The "options" argument must be of type object ... is correct error message instead of The "listener" argument must be of type function. Received type string ('string'). so I fixed it

@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 Aug 1, 2024
Copy link

codecov bot commented Aug 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.09%. Comparing base (5f230d2) to head (b97b1ae).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #54157   +/-   ##
=======================================
  Coverage   87.08%   87.09%           
=======================================
  Files         647      647           
  Lines      181974   181983    +9     
  Branches    34915    34918    +3     
=======================================
+ Hits       158473   158495   +22     
+ Misses      16787    16779    -8     
+ Partials     6714     6709    -5     
Files Coverage Δ
lib/fs.js 93.28% <100.00%> (+0.01%) ⬆️

... and 26 files with indirect coverage changes

@kimtaejin3 kimtaejin3 force-pushed the fix-fs-watchFile branch 3 times, most recently from 1e313f6 to 092b9d1 Compare August 1, 2024 11:04
lib/fs.js Outdated
listener = options;
options = null;
options = {};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
options = {};
options = kEmptyObject;

@daeyeon
Copy link
Member

daeyeon commented Aug 1, 2024

Could you add a test by referring to the following?

// Basic usage tests.
assert.throws(
() => {
fs.watchFile('./some-file');
},
{
code: 'ERR_INVALID_ARG_TYPE',
name: 'TypeError'
});
assert.throws(
() => {
fs.watchFile('./another-file', {}, 'bad listener');
},
{
code: 'ERR_INVALID_ARG_TYPE',
name: 'TypeError'
});

@kimtaejin3 kimtaejin3 force-pushed the fix-fs-watchFile branch 4 times, most recently from 9a19dbd to 2535918 Compare August 4, 2024 04:19
lib/fs.js Outdated
options = kEmptyObject;
}

if (!listener) {
Copy link
Member

Choose a reason for hiding this comment

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

IIUC, when null, 0, false, or NaN is passed to the listener, the error message won't print the value properly.

Suggested change
if (!listener) {
if (listener === undefined) {

@kimtaejin3 kimtaejin3 force-pushed the fix-fs-watchFile branch 3 times, most recently from 389b951 to 2b53545 Compare August 10, 2024 01:14
@daeyeon
Copy link
Member

daeyeon commented Aug 12, 2024

@kimtaejin3 Please check linter errors. It looks unnecessary changes are added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

3 participants