Skip to content
This repository has been archived by the owner on Jun 18, 2021. It is now read-only.

support node -r nodereport app.js as well as programmatic use via API #25

Closed
sam-github opened this issue Nov 17, 2016 · 10 comments
Closed

Comments

@sam-github
Copy link
Contributor

ATM, nodereport works better via API, its clumsy if you want automatic hooking, see:

#23 (comment)

On the other hand, if you want to use nodereport to trigger a report manually, just using its public API, you don't want its (quite intrusive) hook mechanisms, which modify exit codes, behaviours, and even steal signals.

In summary, we need easy opt-in for default behaviour, but we also need easy opt-out, and I don't see any way to provide the two via a single require entry point that does not depend on env vars for configuration.

To make both use-cases easy, I suggest:

node -r nodereport/attach app.js

for the opt-in case, and

require("nodereport").triggerReport()

for the opt-out.

I'm OK with the require paths being switched, so node -r nodereport opts-in, and require("nodereport/api") or something of the like just requires the nodereport API without hooks.

Any other suggestions?

@rnchamberlain
Copy link
Contributor

+1, with preference for 'paths being switched' because the API user is more likely to study the docs.

@sam-github do you have any documentation references for the 'module/option' syntax?

@Fishrock123
Copy link
Contributor

Fishrock123 commented Nov 17, 2016

I would prefer it to be as easy to use on an existing script out of the box as possible.

Maybe it's just me but I don't see the code hooks as being quite as valuable (and the feel a bit more invasive). IIRC this is the sort of reason -r exists in the first place.

FWIW tools like dprof have this similar behavior and I feel a previous precedent has already been set by similar modules that act upon requiring.

@sam-github
Copy link
Contributor Author

So you two would prefer

node -r nodereport app.js

for the opt-in case, and

require("nodereport/api").triggerReport()

for opt-out of hooks, with the understanding that this works to get hooks, and have the API:

require("nodereport").triggerReport()

Above is fine by me.

@richardlau
Copy link
Member

I think I'm also in favour of

node -r nodereport app.js

being opt-in and

require("nodereport/api").triggerReport()

for opt-out of hooks.

Since this would be a change in behaviour, would this be semver-major?

@sam-github
Copy link
Contributor Author

sam-github commented Nov 17, 2016 via email

@mhdawson
Copy link
Member

Given the consensus, I'm ok with node -r being opt in by default.

@rnchamberlain
Copy link
Contributor

PR #33 landed.

node -r nodereport app.js now provides the "no application code change" functionality with hooks/signal on by default, and for api users, require("nodereport/api") is available, with hooks/signal off by default.

Not published in npm yet. Currently we are at 1.0.7, should this go up as 1.1.0 or 2.0.0?

@gibfahn
Copy link
Member

gibfahn commented Dec 21, 2016

2.0.0 as it's a semver-major change

@richardlau
Copy link
Member

node-report v2.0.0 (including #33) has been released.

@rnchamberlain
Copy link
Contributor

@sam-github OK to close this now?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants