FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fs: Replace an Error with a deprecation message. by ChALkeR · Pull Request #1982 · nodejs/node · GitHub

/ node Public

fs: Replace an Error with a deprecation message. - #1982

Closed
ChALkeR wants to merge 1 commit into
nodejs:masterfrom
ChALkeR:fs-stream-fix
ConversationCommits (1)ChecksFiles changed
Closed

fs: Replace an Error with a deprecation message.#1982
ChALkeR wants to merge 1 commit into
nodejs:masterfrom
ChALkeR:fs-stream-fix

Conversation

ChALkeR commented Jun 15, 2015

Copy link
Copy Markdown
Member

This fixes a breaking change in commit 353e26e, that was included in a minor version of io.js and broke at least two modules (that were misusing fs.createWriteStream method, but still).

Disclaimer: I am not sure if this is worth fixing, but if it is, this should work.

The two modules I mentioned above were passing a function to fs.createWriteStream, and that was never supported (it was always ignored). In 2.3.0 a more strict options validation was introduced, it started throwing an Error, and that two modules (at least) broke.

ChALkeR commented Jun 15, 2015

Copy link
Copy Markdown
Member Author

Fixes: #1981

mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Jun 15, 2015

ChALkeR commented Jun 15, 2015

Copy link
Copy Markdown
Member Author

Ah, wait a moment. It still doesn't fix this, needs an extra change.

ChALkeR commented Jun 15, 2015

Copy link
Copy Markdown
Member Author

Ok, should be fixed now (for WriteStream).

Though I am not sure if ReadStream needed any changes at all, because it was calling Object.create before, and that should fail if the passed argument is not an object or null.

thefourtheye reviewed Jun 15, 2015
View reviewed changes
Comment thread lib/fs.js Outdated

thefourtheye Jun 15, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This message will be printed whenever this situation arises. Is that the intention here? Normally, it is used like this

var warned = false;
...
    warned = internalUtil.printDeprecationMessage('...', warned);

ChALkeR Jun 15, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks, fixed.

ChALkeR force-pushed the fs-stream-fix branch 2 times, most recently from 950029e to ee895ff Compare June 15, 2015 14:20
thefourtheye reviewed Jun 15, 2015
View reviewed changes
Comment thread lib/fs.js Outdated

thefourtheye Jun 15, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

In the ReadStream block, else is not there. Should we make this consistent?

ChALkeR Jun 15, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

In the ReadStream, Object.create(options || {}) is called since 4d0329e (v1.5.0). I don't think that there is any need to patch that nowdays and re-introduce pre-1.5.0 behavior.

Actually, even failing with an error on anything but a null in that block (if (options === null || typeof options !== 'object')) should not break anything (compared to v1.5.0). It might be better to add a special case for a null instead of patching that block. What do you think?

thefourtheye Jun 15, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sorry, I meant, options = Object.create(options); used to be outside the if..else if blocks in both the cases. Now it is only inside the else part. Isn't this wrong?

thefourtheye reviewed Jun 15, 2015
View reviewed changes
Comment thread lib/fs.js

thefourtheye Jun 15, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Lets say I pass a string as an argument to ReadStream, then the options will have encoding in its prototype. But in the WriteStream, encoding will be on the options object itself. Is this okay?

ChALkeR Jun 17, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you post a code sample? I think that I'm missing the idea here.

Fishrock123 commented Jun 15, 2015

Copy link
Copy Markdown
Contributor

I would rather revert, than re-land the original on next (next major) once reviewed.

ChALkeR commented Jun 15, 2015

Copy link
Copy Markdown
Member Author

@Fishrock123 That would break a feature that was already released in 2.3.0.

Fishrock123 commented Jun 15, 2015

Copy link
Copy Markdown
Contributor

Ack I see now.

I'm not sure if we should throw a deprecation here or just silently fail not do anything like before. cc @nodejs/tsc

ChALkeR commented Jun 15, 2015

Copy link
Copy Markdown
Member Author

It hasn't failed before, the function argument was just ignored. At least for the WriteStream.
And in 2.3.0 it started throwing.

trevnorris reviewed Jun 15, 2015
View reviewed changes
Comment thread lib/fs.js Outdated

trevnorris Jun 15, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

can drop this an the other commented type error lines.

ChALkeR Jun 17, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed.

yosuke-furukawa mentioned this pull request Jun 16, 2015
Closed
fs: Replace an Error with a deprecation message.
8fd60f4 
This fixes a breaking change in commit
353e26e.
ChALkeR mentioned this pull request Jun 17, 2015
Closed
bricss mentioned this pull request Jun 17, 2015
Closed

ChALkeR commented Jun 17, 2015

Copy link
Copy Markdown
Member Author

Ok, as no one (including myself) seems to actually support the idea of handling this (wrong API usage by modules) on the io.js side, I closed these PRs (both #1982 and #1998).
If anyone of @nodejs/collaborators thinks that this or #1998 should be reopened or that there needs to be more discussion on this matter, just leave a message here.

ChALkeR closed this Jun 17, 2015
ChALkeR mentioned this pull request Jun 20, 2015
Closed
gr00nd mentioned this pull request Feb 6, 2024
Open
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

fs Issues and PRs related to the fs subsystem / file system.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL