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

writeFileSync data as Object throws type error despite docs listing Object as a supported type #39152

Closed
JakobJingleheimer opened this issue Jun 25, 2021 · 8 comments
Labels
doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. good first issue Issues that are suitable for first-time contributors.

Comments

@JakobJingleheimer
Copy link
Contributor

  • Version: 14.17.0 & 16.4.0
  • Platform: local 20.5.0 Darwin Kernel Version 20.5.0: Sat May 8 05:10:31 PDT 2021; root:xnu-7195.121.3~9/RELEASE_ARM64_T8101 arm64
  • Subsystem: FS / Docs

What steps will reproduce the bug?

import { writeFileSync } from 'fs';

writeFileSync('test.log', { foo: 'bar' });

How often does it reproduce? Is there a required condition?

100% (no special conditions)

What is the expected behavior?

The type(s) for data listed in the docs to work.

What do you see instead?

TypeError [ERR_INVALID_ARG_TYPE]: The "data" argument must be of type string or an
instance of Buffer, TypedArray, or DataView. Received an instance of Object
    at writeFileSync (node:fs:2136:5)

Additional information

fs docs for 14.x and 16.x both include <Object> as a type for writeFileSync's data argument:

data <string> | <Buffer> | <TypedArray> | <DataView> | <Object>

@mscdex mscdex added doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. good first issue Issues that are suitable for first-time contributors. labels Jun 25, 2021
@mscdex
Copy link
Contributor

mscdex commented Jun 25, 2021

The object has to have a toString() function on the object itself and not one inherited via prototype.

Unfortunately it seems the documentation did not get updated properly for every API that was touched by these changes (#34993).

@chloebrett
Copy link

chloebrett commented Jun 27, 2021

I think the current documentation is sufficient. The docs for writeFileSync (link) specifies:

For detailed information, see the documentation of the asynchronous version of this API: [`fs.writeFile()`][].

And for writeFile (link):

If `data` is a normal object, it must have an own `toString` function property.

If we added this line to every function with similar parameters to writeFile, then we'd need to add all the other notes as well:

When `file` is a filename, asynchronously writes data to the file, replacing the
file if it already exists. `data` can be a string or a buffer.

When `file` is a file descriptor, the behavior is similar to calling
`fs.write()` directly (which is recommended). See the notes below on using
a file descriptor.

The `encoding` option is ignored if `data` is a buffer.
If `data` is a normal object, it must have an own `toString` function property.

Makes more sense to me to have the specifics detailed under writeFile and direct people to them for all the related functions, which is what we currently do.

@JakobJingleheimer
Copy link
Contributor Author

Ah yes. I think the asterisk as @mscdex described should be added. Otherwise it feels like Gryzzl's T&Cs.

The object needing to have an own toString() method is a pretty big gotcha as it is.

@chloebrett
Copy link

@JakobJingleheimer definitely agree it could be clearer - where would this be added though?

@JakobJingleheimer
Copy link
Contributor Author

JakobJingleheimer commented Jun 27, 2021

I think the various (writeFile, writeFileSync, etc) type defs should all have the asterisk, and it would be fine for the related caveat to be detailed in the one place.

I can put together a PR for that if acceptable 🙂

@chloebrett
Copy link

chloebrett commented Jun 27, 2021

Sure thing. (I'm not an official contributor/maintainer btw, just someone who stumbled across this issue)

While you're at it might be worth doing the same for writeSync as well, as it's the same story

@JakobJingleheimer
Copy link
Contributor Author

Sure! I was thinking to add the asterisk for all related to this (which already link back to the 1 detailed explanation).

Incoming

JakobJingleheimer added a commit to JakobJingleheimer/node that referenced this issue Jun 27, 2021
JakobJingleheimer added a commit to JakobJingleheimer/node that referenced this issue Jun 29, 2021
JakobJingleheimer added a commit to JakobJingleheimer/node that referenced this issue Jun 29, 2021
JakobJingleheimer added a commit to JakobJingleheimer/node that referenced this issue Jun 29, 2021
JakobJingleheimer added a commit to JakobJingleheimer/node that referenced this issue Jul 7, 2021
targos pushed a commit that referenced this issue Jul 11, 2021
Fixes: #39152

PR-URL: #39167
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this issue Sep 4, 2021
Fixes: #39152

PR-URL: #39167
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. good first issue Issues that are suitable for first-time contributors.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@mscdex @JakobJingleheimer @chloebrett and others