Skip to content

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Dec 21, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

fs

Description of change

Allow fs.read, fs.write and fs.writeFile to take Uint8Array arguments.

/cc @nodejs/fs

CI: https://ci.nodejs.org/job/node-test-commit/6765/

Allow `fs.read`, `fs.write` and `fs.writeFile` to take
`Uint8Array` arguments.
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Dec 21, 2016
@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 21, 2016
@targos
Copy link
Member

targos commented Dec 21, 2016

Does isUint8Array return true for a class X extends Uint8Array {} or a future class Y extends Buffer {} ?

@addaleax
Copy link
Member Author

Does isUint8Array return true for a class X extends Uint8Array {} or a future class Y extends Buffer {} ?

Yes and yes; isUint8Array doesn’t look at the prototype like instanceof does, so it won’t care for classes or anything like that.

@ChALkeR
Copy link
Member

ChALkeR commented Dec 21, 2016

Sorry, I'm not able to review this right now, but I wanted to express a heavy +1 for this change.
Thanks for doing this!

@addaleax
Copy link
Member Author

Landed in f2ef850

@addaleax addaleax closed this Dec 26, 2016
@addaleax addaleax deleted the fs-uint8array-input branch December 26, 2016 09:14
addaleax added a commit that referenced this pull request Dec 26, 2016
Allow `fs.read`, `fs.write` and `fs.writeFile` to take
`Uint8Array` arguments.

PR-URL: #10382
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
@evanlucas
Copy link
Contributor

@addaleax this lands cleanly on v7.x but the cherry-pick isn't bring over the changes to src/node_util.cc for some reason? I keep getting this error when trying to run tests:

/Applications/Xcode.app/Contents/Developer/usr/bin/make -C out BUILDTYPE=Release V=1
  touch bac8a0cc7a87f2a1a9ebecffed6451bc8060f038.intermediate
  LD_LIBRARY_PATH=/Users/evan/dev/code/forks/WORK-node/out/Release/lib.host:/Users/evan/dev/code/forks/WORK-node/out/Release/lib.target:$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; cd ../deps/v8_inspector/src/inspector; mkdir -p /Users/evan/dev/code/forks/WORK-node/out/Release/obj/gen/src/inspector/protocol /Users/evan/dev/code/forks/WORK-node/out/Release/obj/gen/include/inspector; python ../../third_party/WebKit/Source/platform/inspector_protocol/CodeGenerator.py --jinja_dir ../../third_party --output_base "/Users/evan/dev/code/forks/WORK-node/out/Release/obj/gen/src/inspector" --config inspector_protocol_config.json
rm bac8a0cc7a87f2a1a9ebecffed6451bc8060f038.intermediate
if [ ! -r node -o ! -L node ]; then ln -fs out/Release/node node; fi
/Applications/Xcode.app/Contents/Developer/usr/bin/make build-addons
/Applications/Xcode.app/Contents/Developer/usr/bin/make -C out BUILDTYPE=Release V=1
  touch bac8a0cc7a87f2a1a9ebecffed6451bc8060f038.intermediate
  LD_LIBRARY_PATH=/Users/evan/dev/code/forks/WORK-node/out/Release/lib.host:/Users/evan/dev/code/forks/WORK-node/out/Release/lib.target:$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; cd ../deps/v8_inspector/src/inspector; mkdir -p /Users/evan/dev/code/forks/WORK-node/out/Release/obj/gen/src/inspector/protocol /Users/evan/dev/code/forks/WORK-node/out/Release/obj/gen/include/inspector; python ../../third_party/WebKit/Source/platform/inspector_protocol/CodeGenerator.py --jinja_dir ../../third_party --output_base "/Users/evan/dev/code/forks/WORK-node/out/Release/obj/gen/src/inspector" --config inspector_protocol_config.json
rm bac8a0cc7a87f2a1a9ebecffed6451bc8060f038.intermediate
if [ ! -r node -o ! -L node ]; then ln -fs out/Release/node node; fi

Building addon /Users/evan/dev/code/forks/WORK-node/test/addons/01_callbacks/
fs.js:627
  if (!isUint8Array(buffer)) {
       ^

TypeError: isUint8Array is not a function
    at Object.fs.readSync (fs.js:627:8)
    at tryReadSync (fs.js:457:20)
    at Object.fs.readFileSync (fs.js:486:19)
    at Object.Module._extensions..js (module.js:579:20)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.runMain (module.js:605:10)
    at run (bootstrap_node.js:420:7)
    at startup (bootstrap_node.js:139:9)
make[1]: *** [test/addons/.buildstamp] Error 1
make: *** [test] Error 2

Mind submitting a backport PR for it?

@evanlucas
Copy link
Contributor

Ah scratch that, looks like it depends on #10236 which is semver-major.

addaleax added a commit to addaleax/node that referenced this pull request Jan 3, 2017
Allow `fs.read`, `fs.write` and `fs.writeFile` to take
`Uint8Array` arguments.

PR-URL: nodejs#10382
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
@addaleax
Copy link
Member Author

addaleax commented Jan 3, 2017

Mind submitting a backport PR for it?

Done: #10593

evanlucas added a commit that referenced this pull request Jan 4, 2017
Notable changes:

* buffer:
  - Improve performance of Buffer allocation by ~11% (Brian White) #10443
  - Improve performance of Buffer.from() by ~50% (Brian White) #10443
* events: Improve performance of EventEmitter.once() by ~27% (Brian White) #10445
* fs: Allow passing Uint8Array to fs methods where Buffers are supported. (Anna Henningsen) #10382
* http: Improve performance of http server by ~7% (Brian White) #6533
* npm: Upgrade to v4.0.5 (Kat Marchán) #10330

PR-URL: #10589
evanlucas added a commit that referenced this pull request Jan 4, 2017
Notable changes:

* buffer:
  - Improve performance of Buffer allocation by ~11% (Brian White) #10443
  - Improve performance of Buffer.from() by ~50% (Brian White) #10443
* events: Improve performance of EventEmitter.once() by ~27% (Brian White) #10445
* fs: Allow passing Uint8Array to fs methods where Buffers are supported. (Anna Henningsen) #10382
* http: Improve performance of http server by ~7% (Brian White) #6533
* npm: Upgrade to v4.0.5 (Kat Marchán) #10330

PR-URL: #10589
imyller added a commit to imyller/meta-nodejs that referenced this pull request Mar 2, 2017
    Notable changes:

    * buffer:
      - Improve performance of Buffer allocation by ~11% (Brian White) nodejs/node#10443
      - Improve performance of Buffer.from() by ~50% (Brian White) nodejs/node#10443
    * events: Improve performance of EventEmitter.once() by ~27% (Brian White) nodejs/node#10445
    * fs: Allow passing Uint8Array to fs methods where Buffers are supported. (Anna Henningsen) nodejs/node#10382
    * http: Improve performance of http server by ~7% (Brian White) nodejs/node#6533
    * npm: Upgrade to v4.0.5 (Kat Marchán) nodejs/node#10330

    PR-URL: nodejs/node#10589

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants