fs: move SyncWriteStream to internal/fs by jasnell · Pull Request #6749 · nodejs/node · GitHub

Checklist

tests and code linting passes
documentation is changed or added
the commit message follows commit guidelines

Affected core subsystem(s)

fs

Description of change

Move the internal SyncWriteStream class to internal/fs. This pulls it off require('fs') without a deprecation. It was always intended to be private, says that it's private in the source and tells people not to use it.

Update the impl a bit while we're at it to use class instead of util.extends

CI: https://ci.nodejs.org/job/node-test-pull-request/2642/

const?

I'd rather not in this PR. There are several similar updates that can be made elsewhere in this file that should be done in a separate PR

perhaps and will be removed soon/in Node.js v***?

Because domain and fs.existsSync are also deprecated 😉.

Perhaps just will be removed soon without a firm version.

@nodejs/ctc ... Tagging as ctc-agenda because it's (a) a semver-major and (b) requires a deprecation. I don't think this one is particularly controversial tho.

+1, but only after a full deprecation cycle

Ah, also +1 from me (with a deprecation cycle).

https://gist.github.com/ChALkeR/f2cb74429769e91752d4b7f41b70dda2 — .SyncWriteStream usage.

I'll copy my comment from a note above:

debug just copied a part of Node.js code, and acknowledges that it's a hack: https://github.com/visionmedia/debug/blob/master/node.js#L137-L142.

I reported an issue there.

mock-fs does Node.js version detection and includes a per-major-version copy of the internal fs module (presumeably patched). We can ignore that.

I suspect some sort of automatic deps packaging for browser as the main culpit for most of the other cases. meteor-please-and-thank-you, for example, has pakmanaged.js which was created by pakmanager and actually includes debug sources. jscs was browserified and also includes debug sources.

The deprecation is in there now. Just squashed the commits down.
New CI: https://ci.nodejs.org/job/node-test-pull-request/2649/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/thealphanerd-smoker/267/

CI is green.
CITGM is green except for known unrelated failure.

@jasnell that serialport failure on ppc is new... rerunning on master to double check

edit: looks like there was an update to serialport that may have broken this

hmm... I've seen that serialport failure before. I believe I was running citgm locally tho.

Note to self, this doesn't need to be done. But for future optimization.

A run on master has serialport tests passing on ppc

Here's another run of this pull's head just to make sure it wasn't a one off: https://ci.nodejs.org/view/Node.js-citgm/job/thealphanerd-smoker/269/

edit: was green this time... ¯_(ツ)_/¯

Happy to see this out of fs.js, LGTM.

This is blocked by #6413, btw. I mean — merging this will obviously break reevaluating fs module sources and old graceful-fs versions, so we should make sure that we are fine with that first.

grrr... {I'm saying a few bad words silently to myself right now}... you're right tho @ChALkeR . Would very much like to get #6413 landed.

I'm curious — aside from the comment that notes that it was supposed to be private, what's the reasoning behind making this private now? It seems like it might not be worth potentially breaking things to move this into internal — I don't think we see much support load for this API as it stands.

CTC discussion on this was to leave this open but hold off on landing until a later date after #6413 lands.

It's a semver-patch that depends on a semver-major, right? Adding the "dont-land" labels.

I believe so, yes, landing on v6 should be fine tho as I believe the semver-major landed there. Could be wrong.

The require('internal/fs'); makes it only landable on master I believe.

ah yes, because of the graceful-fs warning. You're right. That's fine, it wouldn't be critical to land this in v6 at all.

@targos ... other than the don't land labels, does this LGTY?

@ChALkeR ... does this still LGTY? The only change is that I removed the runtime deprecation

@jasnell, what does the «doc-only deprecation» mean here? It isn't documented =).

It just means adding the code comment that indicates the intention to
runtime deprecate in v8 or later.

On Friday, September 2, 2016, Сковорода Никита Андреевич <
notifications@github.com> wrote:

@jasnell https://github.com/jasnell, what does the «doc-only
deprecation» mean here? It isn't documented =).

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6749 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eaDPBp4jLK60RFg20HMuI8DARqUYks5ql9qTgaJpZM4Iebtd
.

@jasnell What would be the difference to users between runtime deprecation in v7 and runtime deprecation in v8? Why do we prefer the latter?

Given the issues that have been caused by other runtime deprecations, and
given that there's no pressing need to runtime deprecate this, I'd prefer
to just space it out a bit more. There's no significant need to rush it.

On Friday, September 2, 2016, Сковорода Никита Андреевич <
notifications@github.com> wrote:

@jasnell https://github.com/jasnell What would be the difference to
users between runtime deprecation in v7 and runtime deprecation in v8? Why
do we prefer the latter?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6749 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2ebCRaRgxcHQ2eVTdRjhJzpYiEbYWks5qmBmSgaJpZM4Iebtd
.

@jasnell Ah, ok. LGTM. =)

@jasnell Perhaps that could be included into the release notes, for this and other «documentation-only» deprecations of undocumented functions.

+1 to adding it to the release notes.

Move the implementation of SyncWriteStream to internal/fs.

New CI: https://ci.nodejs.org/job/node-test-pull-request/3926/

Some weird red in that last CI run... trying again: https://ci.nodejs.org/job/node-test-pull-request/3928/

@nodejs/build ... there's definitely something weird happening with arm in CI

Failures in CI are unrelated. Landing!

Move the implementation of SyncWriteStream to internal/fs. PR-URL: #6749 Reviewed-By: Ron Korving <ron@ronkorving.nl> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

Landed in dc72779

How do we install "internal/fs" ?

npm/npm#14579

when I run npm install --save-dev internal/fs I get an error about my github keys and that I cannot read the remote repo. I'm generally getting errors about cannot find module internal/fs -- what's up here?

You don't install internal/fs, this is a bug in graceful-fs, sounds like.

jasnell added the fs Issues and PRs related to the fs subsystem / file system. label May 13, 2016

Fishrock123 reviewed May 14, 2016
View reviewed changes

ChALkeR added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 14, 2016

ChALkeR mentioned this pull request May 14, 2016

RFC: 3.0 stdout moved off of APIs that are pending removal debug-js/debug#280

Closed

ChALkeR reviewed May 15, 2016
View reviewed changes

jasnell added the ctc-agenda label May 15, 2016

jasnell force-pushed the internal-syncwritestream branch from f2f6515 to 90508a0 Compare May 15, 2016 14:07

trevnorris reviewed May 16, 2016
View reviewed changes

ChALkeR added the blocked PRs that are blocked by other issues or PRs. label May 18, 2016

ChALkeR mentioned this pull request May 18, 2016

Revert "fs: add a temporary fix for re-evaluation support" #6413

Closed

2 tasks

rvagg mentioned this pull request May 18, 2016

Node.js Foundation Core Technical Committee (CTC) Meeting 2016-05-18 #6839

Closed

jasnell removed the ctc-agenda label May 18, 2016

targos added dont-land-on-v4.x labels Sep 1, 2016

fs: move SyncWriteStream to internal/fs

7fbd77c

Move the implementation of SyncWriteStream to internal/fs.

jasnell force-pushed the internal-syncwritestream branch from 818f2e4 to 7fbd77c Compare September 2, 2016 14:21

jasnell closed this Sep 2, 2016

This was referenced Dec 26, 2016

process: Add code to warnings, assign codes to deprecations #10116

Closed

fs: runtime deprecation for fs.SyncWriteStream #10467

Closed

This was referenced Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

test: add internal/fs tests #12306

Closed

fs: move SyncWriteStream to internal/fs by jasnell · Pull Request #6749 · nodejs/node · GitHub

Uh oh!

Uh oh!

fs: move SyncWriteStream to internal/fs - #6749

fs: move SyncWriteStream to internal/fs#6749jasnell wants to merge 1 commit intonodejs:masternodejs/node:masterfrom jasnell:internal-syncwritestreamjasnell/node:internal-syncwritestreamCopy head branch name to clipboard

Conversation

jasnell commented May 13, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

jasnell commented May 13, 2016

Uh oh!

Fishrock123 May 14, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell May 14, 2016

Choose a reason for hiding this comment

Uh oh!

ChALkeR May 15, 2016

Choose a reason for hiding this comment

Uh oh!

ChALkeR May 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell May 15, 2016

Choose a reason for hiding this comment

Uh oh!

jasnell commented May 15, 2016

Uh oh!

rvagg commented May 15, 2016

Uh oh!

ChALkeR commented May 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented May 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented May 16, 2016

Uh oh!

MylesBorins commented May 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented May 16, 2016

Uh oh!

trevnorris May 16, 2016

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented May 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronkorving commented May 18, 2016

Uh oh!

ChALkeR commented May 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented May 18, 2016

Uh oh!

chrisdickinson commented May 18, 2016

Uh oh!

jasnell commented May 18, 2016

Uh oh!

targos commented Sep 1, 2016

Uh oh!

jasnell commented Sep 1, 2016

Uh oh!

targos commented Sep 1, 2016

Uh oh!

jasnell commented Sep 1, 2016

Uh oh!

jasnell commented Sep 1, 2016

Uh oh!

jasnell commented Sep 1, 2016

Uh oh!

ChALkeR commented Sep 2, 2016

Uh oh!

jasnell commented Sep 2, 2016

Uh oh!

ChALkeR commented Sep 2, 2016

fs: move SyncWriteStream to internal/fs#6749
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:internal-syncwritestream

ChALkeR May 15, 2016 •
edited
Loading

ChALkeR commented May 15, 2016 •
edited
Loading

jasnell commented May 15, 2016 •
edited
Loading

MylesBorins commented May 16, 2016 •
edited
Loading

MylesBorins commented May 16, 2016 •
edited
Loading

ChALkeR commented May 18, 2016 •
edited
Loading

ChALkeR commented Sep 2, 2016 •
edited
Loading