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.

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

util: adhere to noDeprecation set at runtime #6683

Closed
wants to merge 1 commit into from
Closed

util: adhere to noDeprecation set at runtime #6683

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented May 11, 2016 •
edited
Loading

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

process (util?)

Description of change

Until now, the docs stated that process.noDeprecation could be set at runtime, but before any modules were loaded. That was not true, because lib/internal/util.js was loaded during the process startup process, so setting the flag at runtime was always pointless.

Minimal test case:

process.noDeprecation = true;
process.EventEmitter;

This patch moves checking process.noDeprecation to the place where it was actually used.

/cc @jasnell

CI: https://ci.nodejs.org/job/node-test-commit/3263/

@addaleax addaleax added the process Issues and PRs related to the process subsystem. label May 11, 2016
bnoordhuis
bnoordhuis reviewed May 11, 2016
View reviewed changes
test/parallel/process-no-deprecation.js
});

const internalUtil = require('internal/util');
internalUtil.printDeprecationMessage('Something is deprecated.');
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a check that calling printDeprecationMessage() after setting process.noDeprecation to false again does emit a deprecation warning?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@bnoordhuis
Copy link
Member

Nice, LGTM.

thefourtheye
thefourtheye reviewed May 11, 2016
View reviewed changes
test/parallel/test-process-no-deprecation.js
const assert = require('assert');

function listener(warning) {
assert.notStrictEqual(warning.name, 'DeprecationWarning');
Copy link
Contributor

Choose a reason for hiding this comment

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

If I am not wrong this function should not be called right? I think assert(false, 'this should not be called'); or something like this would be better I guess.

@thefourtheye thefourtheye added util Issues and PRs related to the built-in util module. and removed process Issues and PRs related to the process subsystem. labels May 11, 2016
@addaleax
Copy link
Member Author

Updated based on @thefourtheye’s suggestion.

Fishrock123
Fishrock123 reviewed May 11, 2016
View reviewed changes
test/parallel/test-process-no-deprecation.js
const assert = require('assert');

function listener(warning) {
assert.fail(null, null, 'received unexpected warning');
Copy link
Contributor

Choose a reason for hiding this comment

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

common.fail(message) :)

@Fishrock123
Copy link
Contributor

Mostly LGTM, good catch.

@addaleax
Copy link
Member Author

@Fishrock123 looking better now? :)

Fishrock123
Fishrock123 reviewed May 11, 2016
View reviewed changes
test/parallel/test-process-no-deprecation.js
const common = require('../common');
const assert = require('assert');

function listener(warning) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused warning?

Copy link
Member Author

Choose a reason for hiding this comment

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

@Fishrock123 eh, yes, sorry. fixed.

@Fishrock123
Copy link
Contributor

Yeah. Another CI would be nice before landing once that nit is fixed

@addaleax addaleax force-pushed the fix-no-deprecation branch 2 times, most recently from fe73664 to 372ea85 Compare May 11, 2016 16:16
jasnell
jasnell reviewed May 11, 2016
View reviewed changes
test/parallel/test-process-no-deprecation.js
const assert = require('assert');

function listener(warning) {
common.fail('received unexpected warning');
Copy link
Member

Choose a reason for hiding this comment

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

this could fail if any other warnings happened to be emitted in the future. The warning object can be checked to see if it is a DeprecationWarning by looking at the name property or the message can be checked explicitly.

Copy link
Member

Choose a reason for hiding this comment

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

That's unlikely though, is it? That would mean core is doing something internally that is worthy of warning. I'd consider that a bug.

Copy link
Member

Choose a reason for hiding this comment

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

Wasn't commenting on the likeliness of it... just the possibility of it. ;-)

Copy link
Member Author

@addaleax addaleax May 11, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@jasnell lol, literally what I had written here at first. But @thefourtheye made a good point, if there are other warnings emitted in the future, it’s probably best to know about that, even if that might mean having to change the test file again.

Copy link
Member

Choose a reason for hiding this comment

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

;-) ok @addaleax ... sgtm

@addaleax
util: adhere to noDeprecation set at runtime
dc8ea89 
Until now, the docs stated that `process.noDeprecation` could be set
at runtime, but before any modules were loaded. That was not true,
because `lib/internal/util.js` was loaded during the process startup
process, so setting the flag at runtime was pointless.

Minimal test case:

    process.noDeprecation = true;
    process.EventEmitter;

This patch moves checking `process.noDeprecation` to the place where
it was actually used.
@addaleax
Copy link
Member Author

New CI: https://ci.nodejs.org/job/node-test-commit/3278/

@thefourtheye
Copy link
Contributor

LGTM

@cjihrig
Copy link
Contributor

cjihrig commented May 13, 2016

LGTM. CI is green.

addaleax added a commit that referenced this pull request May 13, 2016
@addaleax
util: adhere to noDeprecation set at runtime
a4564f3 
Until now, the docs stated that `process.noDeprecation` could be set
at runtime, but before any modules were loaded. That was not true,
because `lib/internal/util.js` was loaded during the process startup
process, so setting the flag at runtime was pointless.

Minimal test case:

    process.noDeprecation = true;
    process.EventEmitter;

This patch moves checking `process.noDeprecation` to the place where
it was actually used.

PR-URL: #6683
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@addaleax
Copy link
Member Author

Landed in a4564f3

@addaleax addaleax closed this May 13, 2016
@addaleax addaleax deleted the fix-no-deprecation branch May 13, 2016 19:35
evanlucas pushed a commit that referenced this pull request May 17, 2016
@addaleax @evanlucas
util: adhere to noDeprecation set at runtime
1d6c17e 
Until now, the docs stated that `process.noDeprecation` could be set
at runtime, but before any modules were loaded. That was not true,
because `lib/internal/util.js` was loaded during the process startup
process, so setting the flag at runtime was pointless.

Minimal test case:

    process.noDeprecation = true;
    process.EventEmitter;

This patch moves checking `process.noDeprecation` to the place where
it was actually used.

PR-URL: #6683
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@evanlucas evanlucas mentioned this pull request May 17, 2016
Merged
evanlucas added a commit that referenced this pull request May 17, 2016
@evanlucas
2016-05-17, Version 6.2.0 (Stable)
9fab6aa 
- **buffer**: fix lastIndexOf and indexOf in various edge cases (Anna
  Henningsen) [#6511](#6511)
- **child_process**: use /system/bin/sh on android (Ben Noordhuis)
  [#6745](#6745)
- **deps**:
  - upgrade npm to 3.8.9 (Rebecca Turner)
    [#6664](#6664)
  - upgrade to V8 5.0.71.47 (Ali Ijaz Sheikh)
    [#6572](#6572)
  - upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé)
    [#6796](#6796)
  - Intl: ICU 57 bump (Steven R. Loomis)
    [#6088](#6088)
- **repl**:
  - copying tabs shouldn't trigger completion (Eugene Obrezkov)
    [#5958](#5958)
  - exports `Recoverable` (Blake Embrey)
    [#3488](#3488)
- **src**: add O_NOATIME constant (Rich Trott)
  [#6492](#6492)
- **src,module**: add --preserve-symlinks command line flag (James M
  Snell) [#6537](#6537)
- **util**: adhere to `noDeprecation` set at runtime (Anna Henningsen)
  [#6683](#6683)
evanlucas added a commit that referenced this pull request May 17, 2016
@evanlucas
2016-05-17, Version 6.2.0 (Stable)
a4705f6 
- **buffer**: fix lastIndexOf and indexOf in various edge cases (Anna
  Henningsen) [#6511](#6511)
- **child_process**: use /system/bin/sh on android (Ben Noordhuis)
  [#6745](#6745)
- **deps**:
  - upgrade npm to 3.8.9 (Rebecca Turner)
    [#6664](#6664)
  - upgrade to V8 5.0.71.47 (Ali Ijaz Sheikh)
    [#6572](#6572)
  - upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé)
    [#6796](#6796)
  - Intl: ICU 57 bump (Steven R. Loomis)
    [#6088](#6088)
- **repl**:
  - copying tabs shouldn't trigger completion (Eugene Obrezkov)
    [#5958](#5958)
  - exports `Recoverable` (Blake Embrey)
    [#3488](#3488)
- **src**: add O_NOATIME constant (Rich Trott)
  [#6492](#6492)
- **src,module**: add --preserve-symlinks command line flag (James M
  Snell) [#6537](#6537)
- **util**: adhere to `noDeprecation` set at runtime (Anna Henningsen)
  [#6683](#6683)

As of this release the 6.X line now includes 64-bit binaries for Linux
on Power Systems running in big endian mode in addition to the existing
64-bit binaries for running in little endian mode.

PR-URL: #6810
evanlucas added a commit that referenced this pull request May 17, 2016
@evanlucas
2016-05-17, Version 6.2.0 (Stable)
26120e2 
- **buffer**: fix lastIndexOf and indexOf in various edge cases (Anna
  Henningsen) [#6511](#6511)
- **child_process**: use /system/bin/sh on android (Ben Noordhuis)
  [#6745](#6745)
- **deps**:
  - upgrade npm to 3.8.9 (Rebecca Turner)
    [#6664](#6664)
  - upgrade to V8 5.0.71.47 (Ali Ijaz Sheikh)
    [#6572](#6572)
  - upgrade libuv to 1.9.1 (Saúl Ibarra Corretgé)
    [#6796](#6796)
  - Intl: ICU 57 bump (Steven R. Loomis)
    [#6088](#6088)
- **repl**:
  - copying tabs shouldn't trigger completion (Eugene Obrezkov)
    [#5958](#5958)
  - exports `Recoverable` (Blake Embrey)
    [#3488](#3488)
- **src**: add O_NOATIME constant (Rich Trott)
  [#6492](#6492)
- **src,module**: add --preserve-symlinks command line flag (James M
  Snell) [#6537](#6537)
- **util**: adhere to `noDeprecation` set at runtime (Anna Henningsen)
  [#6683](#6683)

As of this release the 6.X line now includes 64-bit binaries for Linux
on Power Systems running in big endian mode in addition to the existing
64-bit binaries for running in little endian mode.

PR-URL: #6810
@MylesBorins
Copy link
Contributor

@addaleax lts?

@addaleax
Copy link
Member Author

addaleax commented Jun 2, 2016

Nope, the behaviour in v4.x is fine already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@addaleax @bnoordhuis @Fishrock123 @thefourtheye @cjihrig @MylesBorins @jasnell