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

doc: general improvements to v8.md copy #6829

Closed
wants to merge 3 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented May 18, 2016

Checklist
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

doc (v8)

Description of change

General improvements to v8.md copy.

/cc @nodejs/documentation @bnoordhuis

@jasnell jasnell added doc Issues and PRs related to the documentations. v8 engine Issues and PRs related to the V8 dependency. labels May 18, 2016
@mscdex
Copy link
Contributor

mscdex commented May 18, 2016

/cc @nodejs/v8

`GetHeapSpaceStatistics` function and may change from one V8 version to the
next.

The value returned is an Array of objects containing the following properties:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we usually don't capitalize array in sentences

@jasnell
Copy link
Member Author

jasnell commented May 18, 2016

Nits addressed. PTAL

const v8 = require('v8');
```

The APIs and implementation are subject to change at any time.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It actually means that it can have breaking changes even in minor version bumps, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe so, yes.

@jasnell
Copy link
Member Author

jasnell commented May 18, 2016

@thefourtheye ... updated

@bnoordhuis
Copy link
Member

LGTM

1 similar comment
@targos
Copy link
Member

targos commented May 18, 2016

LGTM

const v8 = require('v8');
```

*Note*: The APIs and implementation are subject to change at any time.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't, and shouldn't be true...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current documentation says:

This module exposes events and interfaces specific to
the version of V8 built with Node.js. These interfaces
are subject to change by upstream and are therefore
not covered under the stability index.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this edit is a restatement of the existing notice, if we want to change this and declare that the v8 module is covered by the stability index, then we can do that in a separate PR.

@thefourtheye
Copy link
Contributor

LGTM

jasnell added a commit that referenced this pull request May 20, 2016
PR-URL: #6829
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented May 20, 2016

Landed in 1b7bb27

@jasnell jasnell closed this May 20, 2016
Fishrock123 pushed a commit that referenced this pull request May 23, 2016
PR-URL: #6829
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
rvagg pushed a commit that referenced this pull request Jun 2, 2016
PR-URL: #6829
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants