Skip to content
This repository has been archived by the owner on Apr 22, 2023. 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
Closed

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

wavded opened this issue Jun 6, 2012 · 5 comments
Labels

Comments

@wavded
Copy link

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
Copy link

isaacs commented Jun 6, 2012

Yeah, that's a good point.

Wanna write a patch? Should be pretty simple.

@wavded
Copy link
Author

wavded commented Jun 6, 2012

Sure can.

@wavded
Copy link
Author

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
Copy link

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
Copy link
Author

wavded commented Jun 7, 2012

Sounds good, thanks @isaacs!

@isaacs isaacs closed this as completed in 569acea Jun 9, 2012
isaacs added a commit to isaacs/node-v0.x-archive that referenced this issue Jun 11, 2012
* 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 nodejs#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 pushed 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.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants