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: Add Documentation Deprecation for process._tickCallback #29781

Closed

Conversation

lholmquist
Copy link
Contributor

Related to #29671.

This starts the documentation deprecation of process._tickCallback

It was suggested that --pending-deprecation could be enabled for this, however, i'm not really sure how to get the value of the --pending-deprecation flag. I've looked at previous commits that do this, but the way they do it, doesn't seem to work in the file that this deprecation needs to happen in. So any guidance there would be great.

Since this API was never documented in the process docs, i didn't add any docs there. If someone feels strongly that docs should be added(and label as deprecated), then i'm open to that.

A remaining question is if this API should be replaced by the suggestion in the other PR, #29671

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added deprecations Issues and PRs related to deprecations. doc Issues and PRs related to the documentations. labels Sep 30, 2019
@jasnell
Copy link
Member

jasnell commented Sep 30, 2019

@nodejs/tsc ... any objections to landing this documentation-only deprecation as a semver-minor?

@Trott
Copy link
Member

Trott commented Sep 30, 2019

@nodejs/tsc ... any objections to landing this documentation-only deprecation as a semver-minor?

I appreciate the ping and feel free to do that in the future if you want, but FYI, no need for TSC approval for landing doc-only deprecations in a minor release. As it says in https://github.com/nodejs/node/blob/2487f390307b28be8c2d1d137f483900937237eb/COLLABORATOR_GUIDE.md#deprecations:

Documentation-Only Deprecations may land in a minor release.

(It's a bit buried in there, suggesting that there might be too much text or something, but it's there!)

@Trott
Copy link
Member

Trott commented Oct 1, 2019

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 1, 2019
@lholmquist
Copy link
Contributor Author

(It's a bit buried in there, suggesting that there might be too much text or something, but it's there!)

sort of a side note here. I wonder if that deprecation section should be broken out into its own doc under doc/guides?

@jasnell
Copy link
Member

jasnell commented Oct 1, 2019

I appreciate the ping and feel free to do that in the future if you want, but FYI, no need for TSC approval for landing doc-only deprecations in a minor release

Yep, I believe I wrote that particular section. Was just a courtesy ping.

@Trott Trott added the notable-change PRs with changes that should be highlighted in changelogs. label Oct 1, 2019
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

I would prefer adding a warning about why and it being risky.

@Trott
Copy link
Member

Trott commented Oct 1, 2019

Yep, I believe I wrote that particular section.

Yep, I'm definitely aware that you wrote the original text. The reason I mentioned it is because it's changed from when you wrote it to remove the semver-major/semver-minor choice. But I didn't want to point that out for some reason. Seemed kind of discourteous somehow but I can't really explain why I was feeling that. Maybe I'm just strange. Anyway...

Original text:

Documentation-Only Deprecations may be handled as semver-minor or semver-major changes.

Current text:

Apply the notable change label to all pull requests that introduce
Documentation-Only Deprecations. Such deprecations have no impact on code
execution. Thus, they are not breaking changes (semver-major).

Speaking of which, notable change label applied! 😄

@lholmquist
Copy link
Contributor Author

@benjamingr i would also like to add a "pending-deprecation" warning if the flag was specificed, but i wasn't sure how to get access to that flag in the file it should happen in, https://github.com/nodejs/node/blob/master/lib/internal/bootstrap/node.js#L287

@addaleax
Copy link
Member

addaleax commented Oct 1, 2019

@lholmquist That sounds good! In that file, CLI flags aren’t available yet (because the results have to be identical across Node.js versions).

You can look for the process.binding deprecation in lib/internal/bootstrap/pre_execution.js and add a second statement to that if, that should take care of it.

@lholmquist
Copy link
Contributor Author

@addaleax thanks. I'll take a look

@lholmquist
Copy link
Contributor Author

@addaleax thanks! that did it

@nodejs-github-bot
Copy link
Collaborator

@Trott Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 2, 2019
@Trott
Copy link
Member

Trott commented Oct 2, 2019

(Added semver-minor because the additional warning with --pending-deprecation is a feature enhancement, or at least could be interpreted as one.)

@Trott
Copy link
Member

Trott commented Oct 2, 2019

Landed in 83418b5

@Trott Trott closed this Oct 2, 2019
Trott pushed a commit that referenced this pull request Oct 2, 2019
This change also supports --pending-deprecation for the new deprecation.

PR-URL: #29781
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@vsemozhetbyt
Copy link
Contributor

Do we replace DEP0XXX on landing?

@richardlau
Copy link
Member

Do we replace DEP0XXX on landing?

In this case #29820 is assigning a code.

I had thought that our process was to assign on landing, but I just checked and it turns out our documented process is for it to happen on release: https://github.com/nodejs/node/blob/e2dcbf1c3231513c38151d729f180a54ea902da9/doc/releases.md#step-3-update-any-replaceme-and-dep00xx-tags-in-the-docs

In any case we have checks in the makefile so release builds will fail if any instances of DEP0XXX remain:

node/Makefile

Lines 937 to 940 in 70c2444

@if [ "$(DISTTYPE)" = "release" ] && \
`grep -q DEP...X doc/api/deprecations.md`; then \
echo 'Please update DEP...X in doc/api/deprecations.md (See doc/releases.md)' ; \
exit 1 ; \

@BridgeAR
Copy link
Member

BridgeAR commented Oct 3, 2019

@richardlau it is documented that it should happen while landing but that's at the very end of the paragraph.

This assignment should occur when the PR is landed, but a check will be made when the release build is run.

So it's mainly a safeguard for the release team.

@richardlau
Copy link
Member

@richardlau it is documented that it should happen while landing but that's at the very end of the paragraph.

This assignment should occur when the PR is landed, but a check will be made when the release build is run.

So it's mainly a safeguard for the release team.

🤦‍♂ You're right, I didn't read far enough. It's late.

BridgeAR pushed a commit that referenced this pull request Oct 9, 2019
This change also supports --pending-deprecation for the new deprecation.

PR-URL: #29781
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Oct 10, 2019
BridgeAR added a commit that referenced this pull request Oct 10, 2019
Notable changes:

* build:
  * Add `--force-context-aware` flag to prevent usage of native node
    addons that aren't context aware
    #29631
* deprecations:
  * Add documentation-only deprecation for `process._tickCallback()`
    #29781
* esm:
  * Using JSON modules is experimental again
    #29754
* fs:
  * Introduce `opendir()` and `fs.Dir` to iterate through directories
    #29349
* process:
  * Add source-map support to stack traces by using
    `--source-map-support` #29564
* tls:
  * Honor `pauseOnConnect` option
    #29635
  * Add option for private keys for OpenSSL engines
    #28973

PR-URL: #29919
BridgeAR added a commit that referenced this pull request Oct 11, 2019
Notable changes:

* build:
  * Add `--force-context-aware` flag to prevent usage of native node
    addons that aren't context aware
    #29631
* deprecations:
  * Add documentation-only deprecation for `process._tickCallback()`
    #29781
* esm:
  * Using JSON modules is experimental again
    #29754
* fs:
  * Introduce `opendir()` and `fs.Dir` to iterate through directories
    #29349
* process:
  * Add source-map support to stack traces by using
    `--source-map-support` #29564
* tls:
  * Honor `pauseOnConnect` option
    #29635
  * Add option for private keys for OpenSSL engines
    #28973

PR-URL: #29919
BridgeAR added a commit that referenced this pull request Oct 11, 2019
Notable changes:

* build:
  * Add `--force-context-aware` flag to prevent usage of native node
    addons that aren't context aware
    #29631
* deprecations:
  * Add documentation-only deprecation for `process._tickCallback()`
    #29781
* esm:
  * Using JSON modules is experimental again
    #29754
* fs:
  * Introduce `opendir()` and `fs.Dir` to iterate through directories
    #29349
* process:
  * Add source-map support to stack traces by using
    `--source-map-support` #29564
* tls:
  * Honor `pauseOnConnect` option
    #29635
  * Add option for private keys for OpenSSL engines
    #28973

PR-URL: #29919
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. deprecations Issues and PRs related to deprecations. doc Issues and PRs related to the documentations. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants