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

fs: move SyncWriteStream to internal/fs #6749

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@jasnell
Member

jasnell commented May 13, 2016

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

@jasnell jasnell added the fs label May 13, 2016

@jasnell

This comment has been minimized.

@Fishrock123

View changes

lib/internal/process/stdio.js Outdated
@@ -131,7 +131,7 @@ function createWritableStdioStream(fd) {
break;
case 'FILE':
var fs = require('fs');
var fs = require('internal/fs');

This comment has been minimized.

@Fishrock123

This comment has been minimized.

@jasnell

jasnell May 14, 2016

Member

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

@Fishrock123

View changes

lib/internal/fs.js Outdated
// Temporary hack for process.stdout and process.stderr when piped to files.
class SyncWriteStream extends Stream {
constructor(fd, options) {

This comment has been minimized.

@Fishrock123

Fishrock123 May 14, 2016

Member

options = {}? Or have we not checked if default params are speedy enough?

(Quite frankly it doesn't matter here though.)

This comment has been minimized.

@jasnell

jasnell May 14, 2016

Member

Default params are not yet fully optimized. However, this class is used at
most once.
On May 14, 2016 1:26 PM, "Jeremiah Senkpiel" notifications@github.com
wrote:

In lib/internal/fs.js
#6749 (comment):

@@ -0,0 +1,73 @@
+'use strict';
+
+const Buffer = require('buffer').Buffer;
+const Stream = require('stream').Stream;
+const fs = require('fs');
+
+function assertEncoding(encoding) {

  • if (encoding && !Buffer.isEncoding(encoding)) {
  • throw new Error('Unknown encoding: ' + encoding);
  • }
    +}

+// Temporary hack for process.stdout and process.stderr when piped to files.
+class SyncWriteStream extends Stream {

  • constructor(fd, options) {

options = {}? Or have we not checked if default params are speedy enough?

(Quite frankly it doesn't matter here though.)


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/pull/6749/files/1173d733a455187d47f4e4eb61a815138e2abfe0#r63281513

This comment has been minimized.

@trevnorris

trevnorris May 16, 2016

Contributor

I would have preferred the code moved in one commit, and converted to a class in another. Makes searching for moved changes easier in git.

This comment has been minimized.

@jasnell

jasnell May 16, 2016

Member

@trevnorris ... ok, but would you consider that critical for getting this landed?

This comment has been minimized.

@trevnorris

trevnorris May 16, 2016

Contributor

No. Just for future reference.

@Fishrock123

View changes

lib/internal/fs.js Outdated
}
// Alias destroySoon -> destroy
SyncWriteStream.prototype.destroySoon = SyncWriteStream.prototype.destroy;

This comment has been minimized.

@Fishrock123

Fishrock123 May 14, 2016

Member

What's this for?

This comment has been minimized.

@jasnell

jasnell May 14, 2016

Member

Not sure. Just copied it straight over from the existing code.
On May 14, 2016 1:27 PM, "Jeremiah Senkpiel" notifications@github.com
wrote:

In lib/internal/fs.js
#6749 (comment):

  •  this.write(data, arg1, arg2);
    
  • }
  • this.destroy();
  • }
  • destroy() {
  • if (this.autoClose)
  •  fs.closeSync(this.fd);
    
  • this.fd = null;
  • this.emit('close');
  • return true;
  • }
    +}

+// Alias destroySoon -> destroy
+SyncWriteStream.prototype.destroySoon = SyncWriteStream.prototype.destroy;

What's this for?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/pull/6749/files/1173d733a455187d47f4e4eb61a815138e2abfe0#r63281523

This comment has been minimized.

@Fishrock123

Fishrock123 May 15, 2016

Member

... Does it appear to work without it?

This comment has been minimized.

@jasnell

jasnell May 15, 2016

Member

It likely would but given that it's part of the existing API, it makes no
sense to remove it yet. Once the public facing API goes through the
deprecation cycle we can look at removing it
On May 15, 2016 8:16 AM, "Jeremiah Senkpiel" notifications@github.com
wrote:

In lib/internal/fs.js
#6749 (comment):

  •  this.write(data, arg1, arg2);
    
  • }
  • this.destroy();
  • }
  • destroy() {
  • if (this.autoClose)
  •  fs.closeSync(this.fd);
    
  • this.fd = null;
  • this.emit('close');
  • return true;
  • }
    +}

+// Alias destroySoon -> destroy
+SyncWriteStream.prototype.destroySoon = SyncWriteStream.prototype.destroy;

... Does it appear to work without it?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/pull/6749/files/1173d733a455187d47f4e4eb61a815138e2abfe0#r63293257

This comment has been minimized.

@jasnell

jasnell May 15, 2016

Member

do a quick search for destroySoon in lib/*.js and you'll see that there are several hits. If you'd like to pull it out, it should be done as a separate PR

@Fishrock123

View changes

lib/fs.js Outdated
@@ -1921,79 +1921,6 @@ WriteStream.prototype.close = ReadStream.prototype.close;
// There is no shutdown() for files.
WriteStream.prototype.destroySoon = WriteStream.prototype.end;
// SyncWriteStream is internal. DO NOT USE.
// Temporary hack for process.stdout and process.stderr when piped to files.

This comment has been minimized.

@Fishrock123

Fishrock123 May 14, 2016

Member

Temporary, eh?

Ideally this would be piped though a net.Socket so that the _handle type would be the same but I don't know if that is feasible.

@Fishrock123

View changes

lib/fs.js Outdated
configurable: true,
writable: true,
value: SyncWriteStream
});

This comment has been minimized.

@Fishrock123

Fishrock123 May 14, 2016

Member

cc @ChALkeR maybe you can check just to be sure no-one's been using this

This comment has been minimized.

@ChALkeR

This comment has been minimized.

@jasnell

jasnell May 14, 2016

Member

Since there is at least some ecosystem use I'll throw a hard deprecation
warning into fs. Thanks for the review @ChALkeR
On May 14, 2016 1:39 PM, "Сковорода Никита Андреевич" <
notifications@github.com> wrote:

In lib/fs.js
#6749 (comment):

  • this.fd = fd;
  • this.writable = true;
  • this.readable = false;
  • this.autoClose = options.autoClose === undefined ? true : options.autoClose;
    -}

-util.inherits(SyncWriteStream, Stream);

-// Export
-Object.defineProperty(fs, 'SyncWriteStream', {

  • configurable: true,
  • writable: true,
  • value: SyncWriteStream
    -});

@Fishrock123 https://github.com/Fishrock123 Done:
https://gist.github.com/ChALkeR/f2cb74429769e91752d4b7f41b70dda2


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/pull/6749/files/1173d733a455187d47f4e4eb61a815138e2abfe0#r63281703

This comment has been minimized.

@Fishrock123

Fishrock123 May 14, 2016

Member

I have to wonder if it isn't actually useful in those cases? Why would someone ever use this if something didn't specifically need it? idk..

This comment has been minimized.

@jasnell

jasnell May 14, 2016

Member

The "don't use this" warning, the clear indication of it being a hack, the
fact that it was never documented, the fact that it was specifically made
non-enumerable , and the fact that it could be easily duplicated in
userland all point to something that could be easily and justifiably made
internal only and eventually dropped if possible.
On May 14, 2016 2:44 PM, "Jeremiah Senkpiel" notifications@github.com
wrote:

In lib/fs.js
#6749 (comment):

  • this.fd = fd;
  • this.writable = true;
  • this.readable = false;
  • this.autoClose = options.autoClose === undefined ? true : options.autoClose;
    -}

-util.inherits(SyncWriteStream, Stream);

-// Export
-Object.defineProperty(fs, 'SyncWriteStream', {

  • configurable: true,
  • writable: true,
  • value: SyncWriteStream
    -});

I have to wonder if it isn't actually useful in those cases? Why would
someone ever use this if something didn't specifically need it? idk..


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/pull/6749/files/1173d733a455187d47f4e4eb61a815138e2abfe0#r63282524

This comment has been minimized.

@ChALkeR

ChALkeR May 15, 2016

Member

@Fishrock123

  • 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.

@ChALkeR

View changes

lib/fs.js Outdated
value: SyncWriteStream
get: util.deprecate(
() => { return _syncwritestream; },
'fs.SyncWriteStream is deprecated.'),

This comment has been minimized.

@ChALkeR

ChALkeR May 15, 2016

Member

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

This comment has been minimized.

@ChALkeR

ChALkeR May 15, 2016

Member

Because domain and fs.existsSync are also deprecated 😉.

This comment has been minimized.

@jasnell

jasnell May 15, 2016

Member

Perhaps just will be removed soon without a firm version.

@jasnell jasnell added the ctc-agenda label May 15, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented May 15, 2016

@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.

@rvagg

This comment has been minimized.

Member

rvagg commented May 15, 2016

+1, but only after a full deprecation cycle

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented May 15, 2016

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.

@jasnell jasnell force-pushed the jasnell:internal-syncwritestream branch May 15, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented May 15, 2016

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/

@jasnell

This comment has been minimized.

Member

jasnell commented May 16, 2016

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

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented May 16, 2016

@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

@jasnell

This comment has been minimized.

Member

jasnell commented May 16, 2016

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

@trevnorris

View changes

lib/internal/fs.js Outdated
// Change strings to buffers. SLOW
if (typeof data === 'string') {
data = Buffer.from(data, encoding);

This comment has been minimized.

@trevnorris

trevnorris May 16, 2016

Contributor

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

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented May 16, 2016

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... ¯_(ツ)_/¯

@ronkorving

This comment has been minimized.

Contributor

ronkorving commented May 18, 2016

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

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented May 18, 2016

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.

@ChALkeR ChALkeR added the blocked label May 18, 2016

@ChALkeR ChALkeR referenced this pull request May 18, 2016

Closed

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

2 of 2 tasks complete
@jasnell

This comment has been minimized.

Member

jasnell commented May 18, 2016

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.

@chrisdickinson

This comment has been minimized.

Contributor

chrisdickinson commented May 18, 2016

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.

@jasnell

This comment has been minimized.

Member

jasnell commented May 18, 2016

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

@jasnell jasnell removed the ctc-agenda label May 18, 2016

@targos

This comment has been minimized.

Member

targos commented Sep 1, 2016

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

@jasnell

This comment has been minimized.

Member

jasnell commented Sep 1, 2016

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

@targos

This comment has been minimized.

Member

targos commented Sep 1, 2016

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

@jasnell

This comment has been minimized.

Member

jasnell commented Sep 1, 2016

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.

@jasnell

This comment has been minimized.

Member

jasnell commented Sep 1, 2016

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

@jasnell

This comment has been minimized.

Member

jasnell commented Sep 1, 2016

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

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Sep 2, 2016

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

@jasnell

This comment has been minimized.

Member

jasnell commented Sep 2, 2016

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
.

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Sep 2, 2016

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

@jasnell

This comment has been minimized.

Member

jasnell commented Sep 2, 2016

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
.

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Sep 2, 2016

@jasnell Ah, ok. LGTM. =)

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Sep 2, 2016

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

@jasnell

This comment has been minimized.

Member

jasnell commented Sep 2, 2016

+1 to adding it to the release notes.

fs: move SyncWriteStream to internal/fs
Move the implementation of SyncWriteStream to internal/fs.

@jasnell jasnell force-pushed the jasnell:internal-syncwritestream branch to 7fbd77c Sep 2, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented Sep 2, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented Sep 2, 2016

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

@jasnell

This comment has been minimized.

Member

jasnell commented Sep 2, 2016

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

@jasnell

This comment has been minimized.

Member

jasnell commented Sep 2, 2016

Failures in CI are unrelated. Landing!

jasnell added a commit that referenced this pull request Sep 2, 2016

fs: move SyncWriteStream to internal/fs
Move the implementation of SyncWriteStream to internal/fs.

PR-URL: #6749
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
@jasnell

This comment has been minimized.

Member

jasnell commented Sep 2, 2016

Landed in dc72779

@jasnell jasnell closed this Sep 2, 2016

@chadfurman

This comment has been minimized.

chadfurman commented Nov 12, 2016

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?

@sam-github

This comment has been minimized.

Member

sam-github commented Nov 14, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment