fs: support Uint8Array input to methods #10382

Closed
wants to merge 1 commit into
from

Projects

None yet

9 participants

@addaleax
Member
addaleax commented Dec 21, 2016 edited
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/

@addaleax addaleax fs: support Uint8Array input to methods
Allow `fs.read`, `fs.write` and `fs.writeFile` to take
`Uint8Array` arguments.
6928259
@targos
Member
targos commented Dec 21, 2016

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

@addaleax
Member

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.

@targos
targos approved these changes Dec 21, 2016 View changes
@ChALkeR
Member
ChALkeR commented Dec 21, 2016 edited

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
Member

Landed in f2ef850

@addaleax addaleax closed this Dec 26, 2016
@addaleax addaleax deleted the addaleax:fs-uint8array-input branch Dec 26, 2016
@addaleax addaleax added a commit that referenced this pull request Dec 26, 2016
@addaleax addaleax fs: support Uint8Array input to methods
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>
f2ef850
@joyeecheung joyeecheung added a commit to joyeecheung/node that referenced this pull request Jan 2, 2017
@addaleax @joyeecheung addaleax + joyeecheung fs: support Uint8Array input to methods
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>
c1a70b3
@evanlucas
Member

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

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

@addaleax addaleax added a commit to addaleax/node that referenced this pull request Jan 3, 2017
@addaleax addaleax fs: support Uint8Array input to methods
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>
abde764
@addaleax
Member
addaleax commented Jan 3, 2017

Mind submitting a backport PR for it?

Done: #10593

@evanlucas evanlucas added a commit that referenced this pull request Jan 4, 2017
@evanlucas evanlucas 2017-01-04, Version 7.4.0 (Current)
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
5d3dd96
@evanlucas evanlucas referenced this pull request Jan 4, 2017
Merged

v7.4.0 release proposal #10589

@evanlucas evanlucas added a commit that referenced this pull request Jan 4, 2017
@evanlucas evanlucas 2017-01-04, Version 7.4.0 (Current)
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
4760abc
@evanlucas evanlucas added a commit that referenced this pull request Jan 4, 2017
@evanlucas evanlucas 2017-01-04, Version 7.4.0 (Current)
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
b8f6c1f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment