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: deprecate top-level this being bound to module.exports #16878

Closed
wants to merge 2 commits into from
Closed

doc: deprecate top-level this being bound to module.exports #16878

wants to merge 2 commits into from

Conversation

Hackzzila
Copy link
Contributor

@Hackzzila Hackzzila commented Nov 8, 2017

Added using the top-level this as an alternative to module.exports to the list of deprecated APIs.

Fixes: #9623

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

doc

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Nov 8, 2017
@jasnell
Copy link
Member

jasnell commented Nov 8, 2017

Not really sure what this is looking to accomplish. Can you provide some additional context?

@addaleax
Copy link
Member

addaleax commented Nov 8, 2017

@jasnell There was some discussion about this in the linked issue

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM, let’s just hope nobody thinks it’s a good idea to add a runtime deprecation

@jasnell
Copy link
Member

jasnell commented Nov 8, 2017

@addaleax ... yes, but the commit message has none of that context and that context is not clear in the doc update.

@@ -737,6 +737,14 @@ Type: Runtime
internal mechanics of the `REPLServer` itself, and is therefore not
necessary in user space.

<a id="DEP0083"></a>
### DEP0083: Top-level `this` bound to `module.exports`
Copy link
Member

Choose a reason for hiding this comment

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

These should be DEP00XX until it lands.

Copy link
Member

@a0viedo a0viedo left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM but needs a rebase

@joyeecheung
Copy link
Member

Ping @Hackzzila can you rebase this against master? Thanks!

@addaleax
Copy link
Member

ping @Hackzzila again

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 31, 2018
@BridgeAR
Copy link
Member

I am going to land this tomorrow.

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 31, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Feb 1, 2018

Landed in 300f5ce

@BridgeAR BridgeAR closed this Feb 1, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Feb 1, 2018
PR-URL: nodejs#16878
Fixes: nodejs#9623
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
PR-URL: nodejs#16878
Fixes: nodejs#9623
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@tniessen tniessen added deprecations Issues and PRs related to deprecations. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Sep 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecations Issues and PRs related to deprecations. doc Issues and PRs related to the documentations. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

should the fact that this is bound to module.exports be documented?
10 participants