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

lib: deprecate _stream_wrap #26245

Closed
wants to merge 1 commit into
base: master
from

Conversation

@sam-github
Copy link
Member

sam-github commented Feb 21, 2019

Its unused by node, and doesn't have a reasonable use outside of node.

See: #25153
See: #16158

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@sam-github sam-github requested a review from addaleax Feb 21, 2019

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Feb 21, 2019

Runtime deprecations are semver-major so I'm labeling it that. (If there is good reason for this to go in as not-a-breaking-change, it needs TSC sign-off on that.)

@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Feb 21, 2019

Definitely semver-major

@BridgeAR
Copy link
Member

BridgeAR left a comment

A test case would likely also be good.

Show resolved Hide resolved doc/api/deprecations.md Outdated
Show resolved Hide resolved doc/api/deprecations.md Outdated
@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Feb 21, 2019

@BridgeAR can you point me to some examples of how deprecations are tested? I can't find one for sys, linklist, or constants (the other deprecated requires).

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Feb 21, 2019

@sam-github

It should be similar to e.g.,

'use strict';

const common = require('../common');

common.expectWarning('DeprecationWarning', {
  DEP0XXX: '$MESSAGE'
});

require('_stream_wrap');

@sam-github sam-github force-pushed the sam-github:deprecate-_stream_wrap branch from cfbe649 to 8ae932e Feb 21, 2019

@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Feb 21, 2019

@BridgeAR Added test, thanks.

@sam-github sam-github force-pushed the sam-github:deprecate-_stream_wrap branch from 8ae932e to 6976e1e Feb 21, 2019

Show resolved Hide resolved lib/_stream_wrap.js Outdated

@sam-github sam-github force-pushed the sam-github:deprecate-_stream_wrap branch from 6976e1e to 53922c6 Feb 21, 2019

@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Feb 21, 2019

@sam-github sam-github force-pushed the sam-github:deprecate-_stream_wrap branch from 53922c6 to d98fbfe Feb 24, 2019

@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Feb 28, 2019

@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Feb 28, 2019

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Mar 1, 2019

This requires a rebase.

@sam-github sam-github force-pushed the sam-github:deprecate-_stream_wrap branch from d98fbfe to 5293f87 Mar 1, 2019

@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Mar 1, 2019

lib: deprecate _stream_wrap
Its unused by node, and doesn't have a reasonable use outside of node.

See: #25153
See: #16158

@sam-github sam-github force-pushed the sam-github:deprecate-_stream_wrap branch from 5293f87 to a1fb336 Mar 2, 2019

@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Mar 2, 2019

Needs two TSC approvals to land a semver-major. @addaleax, you did the prep for this, want to review?

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Mar 4, 2019

@nodejs/tsc PTAL this still needs another LG due to being semver-major.

@BridgeAR

This comment has been minimized.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Mar 4, 2019

I'll approve this if no one else on TSC steps up to do so, but I'm guessing that any deprecation that involves a file with "stream" in the name calls for a review from @mcollina.

@mcollina
Copy link
Member

mcollina left a comment

LGTM

We can really deprecate all of the modules starting with an underscore.

@sam-github

This comment has been minimized.

Copy link
Member Author

sam-github commented Mar 4, 2019

Landed in c7e628f

@sam-github sam-github closed this Mar 4, 2019

@sam-github sam-github deleted the sam-github:deprecate-_stream_wrap branch Mar 4, 2019

sam-github added a commit that referenced this pull request Mar 4, 2019

lib: deprecate _stream_wrap
Its unused by node, and doesn't have a reasonable use outside of node.

See: #25153
See: #16158

PR-URL: #26245
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

addaleax added a commit to addaleax/node that referenced this pull request Mar 7, 2019

@addaleax addaleax referenced this pull request Mar 7, 2019

Closed

lib: assign missed deprecation code #26492

4 of 4 tasks complete

addaleax added a commit that referenced this pull request Mar 7, 2019

lib: assign missed deprecation code
Refs: #26245

PR-URL: #26492
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.