This is a fix for #7655. @micnic @thealphanerd This can be safely backported, if necessary.

LGTM with some comments

As I see, in some fs stream prototype constructors Object.create() is used instead of utils._extend(), it would be good to use a single object properties copying method

About tests, it would be good to have such tests for other methods that receive as parameters user defined objects, cc @nodejs/testing

@micnic @cjihrig Comments addressed. PTAL.

LGTM

cc @nodejs/collaborators

LGTM

CI: https://ci.nodejs.org/job/node-test-pull-request/3456/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/thealphanerd-smoker-xunit/41/

Object.assign might be better than util._extend which would be deprecated by the former one day?

@yorkie from #7208 and #7255 it was decided to use the util._extend() as it has better performance than Object.assign()

@thefourtheye Why does this remove Object.create() from tests? Does it break with Object.create()?

> Object.create(Object.create({encoding: 'utf8'})).encoding
'utf8'
> util._extend({}, Object.create({encoding: 'utf8'})).encoding
undefined

Doesn't it make this semver-major?

Why does this remove Object.create() from tests? Does it break with Object.create()?

@ChALkeR because the Object.create takes its arguments into prototype chain, not the direct field, but the Object.keys that the util._extend is internally using does not return keys from prototype chain as below:

The Object.keys() method returns an array of a given object's own enumerable properties, in the same order as that provided by a for...in loop (the difference being that a for-in loop enumerates properties in the prototype chain as well).

@yorkie, so, this means that it actually broke those tests?

so, this means that it actually broke those tests?

Yea, we have to change those cases IMO :-(

Labeling as semver-major due to a breaking change. Note that it also changes existing tests expectations.

Feel free to remove the semver-major label if this will be changed so that all the existing tests pass.

Could we keep support for those somehow? Btw, this potentially could break some modules out there, so I'm not even sure if landing this in the current state to 7.0 without a deprecation cycle will be fine. Refs: #7912.

Note: it's not semver-major due to the actual intended change that is described in the commit title, it's semver-major because passing something like Object.create({encoding: 'utf8'}) worked before and doesn't work now.

@nodejs/ctc ... any further thoughts on this?
@thefourtheye ... the PR needs to be rebased and updated.

What is this options = Object.create(options) trying to achieve anyways?

@jasnell I think I am blocked by #8006. I'll rebase and update the PR once I can test this locally.

@silverwind We add few properties to options object passed by users, in few of the fs APIs. Object.create(options) would avoid mutating the original options object passed by user.

@thefourtheye Wouldn't something like:

const options = { a: { b: 1 } };
const newOptions = Object.create(options);
newOptions.a.b = 2;

still alter the original options object?

The status post must be wrong as there was a failure on aix even though the check above show as green. I assume this was not seen earlier as the test did not make it that far.

not ok 338 parallel/test-fs-options-immutable
# events.js:160
#       throw er; // Unhandled 'error' event
#       ^
# 
# Error: ENOENT: no such file or directory, open '/home/iojs/node-tmp/tmp.0/streams'
  ---

Hmm, see it passed in an earlier CI run so not sure why that would fail unless there could be some conflict with temp directory generation and other tests running in parallel ?

Failure on PPC was unrelated as its related to the know test-tic-processor-issues covered in: #8725

Stress run on AIX https://ci.nodejs.org/job/node-stress-single-test/986/

Seems to fail consistently in stress run, did the test change since the successful CI run ?

Investigating locally, believe it may be because the test assumes the write will complete before the read starts which is not guaranteed

Yes believe it is a race condition in the test. This version passes reliably:

{
  const fileName = path.resolve(common.tmpDir, 'streams');
  assert.doesNotThrow(() =>
    fs.WriteStream(fileName, options).once('open', () => {
      assert.doesNotThrow(() => fs.ReadStream(fileName, options))
    })
  );
}

Awesome. Thanks for digging deeper and providing a fix as well, @mhdawson :-)

https://ci.nodejs.org/job/node-stress-single-test/989/ is Green.

One last CI Run before landing: https://ci.nodejs.org/job/node-test-pull-request/4437/

I had to rebase because of the linter rule upgrade. New CI: https://ci.nodejs.org/job/node-test-pull-request/4440/

FreeBSD failures are not related. Landing this now...

Landed in 7542bdd

This does not land cleanly on v7.x-staging without the semver-major #7165 also applied.

@jasnell Oh, do you want me to PR this targeting 7.x?

No no, sorry i wasn't clear. Both prs landed in v7.x-staging, I was just
documenting the dependency between them.

On Monday, October 10, 2016, Sakthipriyan Vairamani <
notifications@github.com> wrote:

@jasnell https://github.com/jasnell Oh, do you want me to PR this
targeting 7.x?

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

Backport PRs will be necessary if desired, this appears to depend on #7165 / I don't know how to resolve easily.