Skip to content

buffer: deprecate parent property #8332

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

buffer: deprecate parent property #8332

wants to merge 2 commits into from

Conversation

@thefourtheye
Copy link
Contributor

@thefourtheye thefourtheye commented Aug 30, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

buffer

Description of change

The buffer property already exposes the underlying buffer object. So
this property is being deprecated.

Refer: #8266
Refer: #8311

cc @nodejs/buffer

@thefourtheye thefourtheye added buffer Issues and PRs related to the buffer subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Aug 30, 2016
@thefourtheye thefourtheye force-pushed the buffer-deprecate-parent branch from 0ee50f4 to 81fdc17 Compare August 30, 2016 06:30
@addaleax
Copy link
Member

addaleax commented Aug 30, 2016

Hmm I don’t want to hijack #7964 with a discussion of this specific issue, so I’m commenting here.
I’m basically agreeing with what @bnoordhuis said over there, so far I don’t see any good reason for a runtime deprecation.
We’re in a bit of a pickle here in that a docs-only deprecation is hard because, um, there are no docs on this… I guess my suggestion would be just leaving it undocumented.

@trevnorris
Copy link
Contributor

trevnorris commented Sep 8, 2016

I find it a bit annoying b/c it's undocumented, and untested. Honestly we might as well just set buf.parent = buf.buffer and be done w/ it.

@rvagg rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02
@thefourtheye
Copy link
Contributor Author

thefourtheye commented Jan 10, 2017

@nodejs/ctc How do we feel about just doc-deprecating this, based on #7964 (comment)?

An in my opinion valid reason for deprecating buffer.parent - in the documentation - is that there should be a canonical way of doing things. It lowers friction for new users and eases maintenance for existing users and is generally always a good thing. That's why I think it's proper for the documentation to say "don't use this thing, use that."

@jasnell
Copy link
Member

jasnell commented Jan 10, 2017

I'm good with a docs-only deprecation

@thefourtheye
doc: document buffer.buffer property
930d1d7 
Buffer objects expose the underlying `Uint8Array`'s `buffer` property
by default. This patch formally documents it.
@thefourtheye thefourtheye force-pushed the buffer-deprecate-parent branch from 81fdc17 to e693c92 Compare January 10, 2017 19:32
@thefourtheye thefourtheye force-pushed the buffer-deprecate-parent branch from e693c92 to f993ee6 Compare January 10, 2017 19:37
@thefourtheye
Copy link
Contributor Author

thefourtheye commented Jan 10, 2017

Updated the PR with docs-only deprecation. PTAL.

jasnell
jasnell reviewed Jan 10, 2017
View reviewed changes
doc/api/buffer.md Outdated
Copy link
Member

@jasnell jasnell Jan 10, 2017

Choose a reason for hiding this comment

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

Might add here a quick comment saying something like:

The `buf.parent` property is a deprecated alias for `buf.buffer`.

Copy link
Contributor Author

@thefourtheye thefourtheye Jan 13, 2017

Choose a reason for hiding this comment

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

Ack!

@thefourtheye
doc: deprecate buffer's parent property
a4a43ac 
`buffer.parent` property is actually a wrapper over `buffer.buffer`
property. This patch actually doc-deprecates it and points the users to
the `buffer.buffer` property.
@thefourtheye thefourtheye force-pushed the buffer-deprecate-parent branch from f993ee6 to a4a43ac Compare January 13, 2017 17:23
@thefourtheye
Copy link
Contributor Author

thefourtheye commented Jan 18, 2017

Landed in d708700 and 03d440e

@thefourtheye thefourtheye deleted the buffer-deprecate-parent branch January 18, 2017 06:25
thefourtheye added a commit that referenced this pull request Jan 18, 2017
@thefourtheye
doc: document buffer.buffer property
03d440e 
Buffer objects expose the underlying `Uint8Array`'s `buffer` property
by default. This patch formally documents it.

PR-URL: #8332
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
thefourtheye added a commit that referenced this pull request Jan 18, 2017
@thefourtheye
doc: deprecate buffer's parent property
d708700 
`buffer.parent` property is actually an alias of `buffer.buffer`
property. This patch actually doc-deprecates it and points the users to
the `buffer.buffer` property.

PR-URL: #8332
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
@thefourtheye
doc: document buffer.buffer property
02391a0 
Buffer objects expose the underlying `Uint8Array`'s `buffer` property
by default. This patch formally documents it.

PR-URL: nodejs#8332
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
@thefourtheye
doc: deprecate buffer's parent property
40adf97 
`buffer.parent` property is actually an alias of `buffer.buffer`
property. This patch actually doc-deprecates it and points the users to
the `buffer.buffer` property.

PR-URL: nodejs#8332
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@MylesBorins
Copy link
Contributor

MylesBorins commented Jan 23, 2017

@Fishrock123 bot should not be tagging semver major commits for lts

@jasnell jasnell mentioned this pull request Apr 4, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

+1 more reviewer

@trevnorris trevnorris trevnorris approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

buffer Issues and PRs related to the buffer subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@thefourtheye @addaleax @trevnorris @jasnell @MylesBorins @nodejs-github-bot