Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposal: move node-report into core #18760

Closed
cjihrig opened this issue Feb 13, 2018 · 18 comments
Closed

Proposal: move node-report into core #18760

cjihrig opened this issue Feb 13, 2018 · 18 comments
Labels
meta Issues and PRs related to the general management of the project.

Comments

@cjihrig
Copy link
Contributor

cjihrig commented Feb 13, 2018

The idea of moving node-report was brought up at the Diagnostics Summit.

Thoughts? Objections?

@dshaw
Copy link

dshaw commented Feb 13, 2018

@gireeshpunathil
Copy link
Member

+1.
By moving this into core, will the report generation hooks be enabled by default?

@targos
Copy link
Member

targos commented Feb 13, 2018

+1

2 similar comments
@mmarchini
Copy link
Contributor

+1

@jasnell
Copy link
Member

jasnell commented Feb 13, 2018

+1

@richardlau
Copy link
Member

Some previous discussion in nodejs/node-report#103.

@Fishrock123
Copy link
Contributor

I'd be ok with this but it will need to modifications to default behavior and also perhaps other things.

Also, the api would have to be not exposed as a separate module. I propose we do not attempt to expose the JS api until the other parts are implemented.

@jasnell
Copy link
Member

jasnell commented Feb 13, 2018

I agree with not exposing the js API as a separate module. A function off util or console should work fine. There are definitely a few internal bits that will need to be looked at also.

@devsnek
Copy link
Member

devsnek commented Feb 13, 2018

+1

1 similar comment
@hiroppy
Copy link
Member

hiroppy commented Feb 13, 2018

+1

@joyeecheung
Copy link
Member

joyeecheung commented Feb 14, 2018

@gireeshpunathil

By moving this into core, will the report generation hooks be enabled by default?

I think what we have talked about at the summit was, first putting triggerReport() in util and making it a semver-minor change, the whole thing will be experimental. Then we iterate on that and discuss whether to enable it by default, where it should log to and how to specify that, .etc

@joyeecheung
Copy link
Member

One thing we need to discuss: how is the test CI integration of node-report going to work? Right now it does not have a testing CI I believe. During the summit we talked about vendoring it in like node-inspect, so it would be great if it has separate testing CIs for PRs of its own.

cc @richardlau @rnchamberlain @hhellyer

@rnchamberlain
Copy link

@joyeecheung there is a CI here: https://ci.nodejs.org/view/post-mortem/job/nodereport-continuous-integration/

@richardlau
Copy link
Member

In addition to the CI for testing node-report itself it is also included in CITGM runs.

@hashseed
Copy link
Member

+1

@joyeecheung joyeecheung added the meta Issues and PRs related to the general management of the project. label Feb 20, 2018
gireeshpunathil added a commit to gireeshpunathil/node that referenced this issue Sep 4, 2018
Make node-report part of core runtime, to satisfy its tier1 status
on diagnostic tooling.

No new functionalities have been added, changes that are required for
melding it as a built-in capability has been affected on the module
version of node-report (https://github.com/nodejs/node-report)

Refs: nodejs#19661
Refs: nodejs#18760
Refs: nodejs/node-report#103
gireeshpunathil added a commit to gireeshpunathil/node that referenced this issue Sep 4, 2018
Make node-report part of core runtime, to satisfy its tier1 status
on diagnostic tooling.

No new functionalities have been added, changes that are required for
melding it as a built-in capability has been affected on the module
version of node-report (https://github.com/nodejs/node-report)

Refs: nodejs#19661
Refs: nodejs#18760
Refs: nodejs/node-report#103
gireeshpunathil added a commit to gireeshpunathil/node that referenced this issue Sep 5, 2018
Make node-report part of core runtime, to satisfy its tier1 status
on diagnostic tooling.
No new functionalities have been added, changes that are required for
melding it as a built-in capability has been affected on the module
version of node-report (https://github.com/nodejs/node-report)

Refs: nodejs#19661
Refs: nodejs#18760
Refs: nodejs/node-report#103
gireeshpunathil added a commit to gireeshpunathil/node that referenced this issue Sep 5, 2018
Make node-report part of core runtime, to satisfy its tier1 status
on diagnostic tooling.
No new functionalities have been added, changes that are required for
melding it as a built-in capability has been affected on the module
version of node-report (https://github.com/nodejs/node-report)

Refs: nodejs#19661
Refs: nodejs#18760
Refs: nodejs/node-report#103
@Trott
Copy link
Member

Trott commented Nov 6, 2018

It seems like perhaps this should be closed. Feel free to re-open (or leave a comment requesting that it be re-opened) if you disagree. (Or, perhaps better, open a PR or get the existing one at #22712 moving.) I'm just tidying up and not acting on a super-strong opinion or anything like that.

@Trott Trott closed this as completed Nov 6, 2018
@gireeshpunathil
Copy link
Member

22712 is on track, just that I am low on bandwidth at the moment, went with some open libuv issues that were causing CI failures. 22712 should come to life soon.

@sam-github sam-github reopened this Nov 7, 2018
@gireeshpunathil
Copy link
Member

#22712 is merged now; so closing. fyi - @nodejs/diagnostics

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

No branches or pull requests