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

src: deprecate root and GLOBAL variables #1838

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
@JacksonTian
Copy link
Contributor

commented May 29, 2015

The root and GLOBAL never be documented.

@silverwind

This comment has been minimized.

Copy link
Contributor

commented May 29, 2015

I think these should be pretty low risk to remove. If someone has an idea on how to print a deprecation warning when accessing the global or its properties, we could also go that route.

@Fishrock123

This comment has been minimized.

Copy link
Member

commented May 29, 2015

If someone has an idea on how to print a deprecation warning when accessing the global or its properties

Use a Getter?

@brendanashworth

This comment has been minimized.

Copy link
Member

commented May 29, 2015

There shouldn't be much code relying on this but there may be some so we do need a deprecation message.

edit: btw the commit log should be src: not lib:

@silverwind

This comment has been minimized.

Copy link
Contributor

commented May 29, 2015

Use a Getter?

Won't work for usage like GLOBAL.setTimeout() is suppose.

@Fishrock123

This comment has been minimized.

Copy link
Member

commented May 29, 2015

@silverwind isn't that still accessing GLOBAL from global?

@silverwind

This comment has been minimized.

Copy link
Contributor

commented May 29, 2015

@Fishrock123 right, something like this should work:

Object.defineProperty(global, 'GLOBAL', {get: function() {
  util.deprecate(function() {}, "'GLOBAL' is deprecated, use 'global'");
  return global;
}});

@JacksonTian could you try incorporating such a print to both globals instead of removing them?

@silverwind

This comment has been minimized.

Copy link
Contributor

commented May 29, 2015

Better one:

Object.defineProperty(global, 'GLOBAL', {
    get: util.deprecate(function() { return global; }, "'GLOBAL' is deprecated, use 'global'")
});
@targos

This comment has been minimized.

Copy link
Member

commented May 29, 2015

maybe also make the property writable ?

@silverwind

This comment has been minimized.

Copy link
Contributor

commented May 29, 2015

maybe also make the property writable ?

Right, this is getting interesting now. Below code should only print once on first read/write access:

var g = r = global;
var accessGlobal = util.deprecate(function(val) {
    if (val) g = val;
    return g;
}, "'GLOBAL' is deprecated, use 'global'");
var accessRoot = util.deprecate(function(val) {
    if (val) r = val;
    return r;
}, "'root' is deprecated, use 'global'");

Object.defineProperty(global, 'GLOBAL', {get: accessGlobal, set: accessGlobal});
Object.defineProperty(global, 'root', {get: accessRoot, set: accessRoot});
@silverwind

This comment has been minimized.

Copy link
Contributor

commented May 29, 2015

One more revision:

const holders = {GLOBAL: global, root: global};
const accessGlobal = util.deprecate(function(val) {
    if (val) holders.GLOBAL = val;
    return holders.GLOBAL;
}, "'GLOBAL' is deprecated, use 'global'");
const accessRoot = util.deprecate(function(val) {
    if (val) holders.root = val;
    return holders.root;
}, "'root' is deprecated, use 'global'");

Object.defineProperty(global, 'GLOBAL', {get: accessGlobal, set: accessGlobal});
Object.defineProperty(global, 'root', {get: accessRoot, set: accessRoot});

@JacksonTian feel free to use something like that. Otherwise, I could do a PR that succeeds this one :)

@JacksonTian JacksonTian force-pushed the JacksonTian:remove_root branch 2 times, most recently from acbdbcf to 233d4f8 May 30, 2015

@JacksonTian

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2015

@silverwind The PR is updated, please review it again.

@JacksonTian JacksonTian force-pushed the JacksonTian:remove_root branch from 233d4f8 to e2d8bf4 May 30, 2015

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented May 30, 2015

GLOBAL === global and root === global so you can just return this inside the setter, no need to create a separate object.

Apropos assignment, I'd replace the getter/setter inside the setter, e.g.:

Object.defineProperty(global, 'GLOBAL', {
  configurable: true,  // but not enumerable
  get: util.deprecate(function() { return this; },
                      `'GLOBAL' is deprecated, use 'global'`),
  set: function(value) {
    const desc = { configurable: true, enumerable: true, value: value };
    Object.defineProperty(this, 'GLOBAL', desc);
  },
});

Ditto for root. You can even go one step further and redefine the property inside the getter as well if you change it to:

get: util.deprecate(function() {
  const desc = { configurable: true, enumerable: true, value: this };
  Object.defineProperty(this, 'GLOBAL', desc);
  return this;
}, `'GLOBAL' is deprecated, use 'global'`)

That prints a warning on the first access and then gets out of the way. It won't regress code that uses GLOBAL or root in performance-sensitive code.

@silverwind

This comment has been minimized.

Copy link
Contributor

commented May 30, 2015

+1 for using this.

@bnoordhuis I have a bit of a hard time understanding why running defineProperty each access can be faster, but you're probably right :)

@silverwind silverwind changed the title lib: remove variables root and GLOBAL src: deprecate root and GLOBAL variables May 30, 2015

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented May 30, 2015

@silverwind The getter calls defineProperty on the first access. After that the property is no longer a getter, just a regular property.

@silverwind

This comment has been minimized.

Copy link
Contributor

commented May 30, 2015

@bnoordhuis oh right, thats quite genius!

@targos

This comment has been minimized.

Copy link
Member

commented May 31, 2015

@bnoordhuis 👍 very clever

The setter can be simplified using shorthand property:

set(value) {
  const desc = { configurable: true, enumerable: true, value: value };
  Object.defineProperty(this, 'GLOBAL', desc);
}
@targos

This comment has been minimized.

Copy link
Member

commented May 31, 2015

and maybe it should be writable: true as well ?

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented May 31, 2015

Good point, it should be writable when setting the value (but not when setting the getter/setter pair.)

@JacksonTian JacksonTian force-pushed the JacksonTian:remove_root branch from e2d8bf4 to 63f0d86 Jun 1, 2015

@JacksonTian

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2015

Hi everyone, I updated the PR by your comments, thanks.

src/node.js Outdated
// getter
const get = NativeModule.require('util').deprecate(function() {
const desc = { configurable: true, enumerable: true, value: this };
Object.defineProperty(this, 'GLOBAL', desc);

This comment has been minimized.

Copy link
@Fishrock123

Fishrock123 Jun 1, 2015

Member

Should be Object.defineProperty(this, name, desc);

@ChALkeR

This comment has been minimized.

Copy link
Member

commented Feb 4, 2016

Will post an updated list with full data when I will get to my PC.

@cjihrig

View changes

src/node.js Outdated
// Deprecate GLOBAL and root
['GLOBAL', 'root'].forEach(function(name) {
// getter
const get = NativeModule.require('util').deprecate(function() {

This comment has been minimized.

Copy link
@cjihrig

cjihrig Feb 4, 2016

Contributor

Tiny suggestion: the code might look a little better if you pulled the NativeModule.require('util') out into a variable. Feel free to ignore if you want.

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2016

LGTM with the same suggestions. And +1 to this being semver-major.

@ChALkeR

This comment has been minimized.

Copy link
Member

commented Feb 4, 2016

@jasnell

This comment has been minimized.

Copy link
Member

commented Feb 4, 2016

LGTM as semver-major

@silverwind silverwind removed the stalled label Feb 4, 2016

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2016

@brendanashworth

This comment has been minimized.

Copy link
Member

commented Feb 5, 2016

@bnoordhuis back then, root and GLOBAL were just outright removed, but when they were deprecated I changed it to semver-minor. +1 for major though.

@JacksonTian JacksonTian force-pushed the JacksonTian:remove_root branch Feb 8, 2016

src: deprecate undocumented variables
The `root` and `GLOBAL` never be documented.

@JacksonTian JacksonTian force-pushed the JacksonTian:remove_root branch to 22e123b Feb 8, 2016

@JacksonTian

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2016

Updated. Thanks all.

@thefourtheye

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2016

LGTM

@ChALkeR

This comment has been minimized.

Copy link
Member

commented Feb 8, 2016

LGTM (if passes CI)

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2016

bnoordhuis added a commit that referenced this pull request Feb 10, 2016

src: deprecate undocumented variables
The `root` and `GLOBAL` were never documented.

PR-URL: #1838
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Feb 10, 2016

Unrelated lint error addressed by #5161. Landed in 4e46931, thanks @JacksonTian!

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.