This repository has been archived by the owner. It is now read-only.

[domains] domain.intercept - remove err argument from cb function #3379

Closed
wavded opened this Issue Jun 6, 2012 · 5 comments

Comments

Projects
None yet
2 participants

wavded commented Jun 6, 2012

Just throwing it out there for discussion. Been reviewing domains in 0.7.x and really glad we are getting them! Good work.

One API thing at the moment has bugged a me a few times that I thought it would be worth a discussion regardless of the outcome (and at the very least would help me understand the current design choices).

Right now domain.intercept takes a callback that includes the err argument but from what I see in the code (https://github.com/joyent/node/blob/master/lib/domain.js#L142-177) that error argument will always be null if it makes it that far. Why not remove it entirely to give the user a tip off that intercept is actually taking care of that?

e.g.

var d = domain.create()
d.on('error', function (err) { /* handle errors */ })

fs.readFile('./foo', d.intercept(function (data) {
   // do something with successfully returned data
}));

isaacs commented Jun 6, 2012

Yeah, that's a good point.

Wanna write a patch? Should be pretty simple.

wavded commented Jun 6, 2012

Sure can.

wavded added a commit to wavded/node that referenced this issue Jun 6, 2012

wavded commented Jun 6, 2012

@isaacs made a pull request and it was my first time for this project :) so if I need to change anything or do more or less let me know, tried to follow guidelines on the wiki.

wavded added a commit to wavded/node that referenced this issue Jun 6, 2012

isaacs commented Jun 7, 2012

@wavded I was just about to ask that you sign the CLA, but I see you just did :)

I'll review this soon for the next 0.7 release.

wavded commented Jun 7, 2012

Sounds good, thanks @isaacs!

@isaacs isaacs closed this in 569acea Jun 9, 2012

isaacs added a commit to isaacs/node-v0.x-archive that referenced this issue Jun 11, 2012

2012.06.11, Version 0.7.10 (unstable)
* Roll V8 back to 3.9.24.31

* build: x64 target should always pass -m64 (Robert Mustacchi)

* add NODE_EXTERN to node::Start (Joel Brandt)

* repl: Warn about running npm commands (isaacs)

* slab_allocator: Fix crash in dtor if V8 is dead (Ben Noordhuis)

* slab_allocator: fix leak of Persistent handles (Shigeki Ohtsu)

* windows/msi: add node.js prompt to startmenu (Jeroen Janssen)

* windows/msi: fix adding node to PATH (Jeroen Janssen)

* windows/msi: add start menu links when installing (Jeroen Janssen)

* windows: don't install x64 version into the 'program files (x86)' folder (Matt Gollob)

* domain: Fix #3379 domain.intercept no longer passes error arg to cb (Marc Harter)

* fs: make callbacks run in global context (Ben Noordhuis)

* fs: enable fs.realpath on windows (isaacs)

* child_process: expose UV_PROCESS_DETACHED as options.detached (Charlie McConnell)

* child_process: new stdio API for .spawn() method (Fedor Indutny)

* child_process: spawn().ref() and spawn().unref() (Fedor Indutny)

* Upgrade npm to 1.1.25
- Enable npm link on windows
- Properly remove sh-shim on Windows
- Abstract out registry client and logger

isaacs added a commit to isaacs/node-v0.x-archive that referenced this issue Jun 11, 2012

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