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: move GLOBAL and root aliases to EOL #31167

Open
wants to merge 1 commit into
base: master
from

Conversation

@jasnell
Copy link
Member

jasnell commented Jan 2, 2020

GLOBAL and root have been long deprecated.

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
GLOBAL and root have been long deprecated.
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jan 2, 2020

Might we decide to never EOL this, similar to how we are likely to never remove sys? Isn't the cost of leaving around root and GLOBAL as deprecated aliases for global close to zero, while the cost for the ecosystem of removing them is non-zero?

@devsnek
devsnek approved these changes Jan 2, 2020
@jasnell

This comment has been minimized.

Copy link
Member Author

jasnell commented Jan 2, 2020

The cost of keeping these around is definitely not zero really as it contributes to a significant amount of legacy cruft that bloats the code and makes things more difficult to maintain in the long run. I'm fine with keeping these around longer if there are actively used modules in the ecosystem that still use them; alternatively, we can proactively open PR fixes in any module that does.

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Jan 3, 2020

This should be okay, given that the runtime deprecation effectively already makes these broken, but the maintenance cost here is close to zero, to be fair.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jan 6, 2020

This change causes underscore tests to fail in CITGM because GLOBAL is used in test/object.js in the most recent release of underscore. https://ci.nodejs.org/job/citgm-smoker/2189/testReport/

This issue was fixed by @BridgeAR in March 2019 but that change has yet to make it into a release. jashkenas/underscore@5304f86

@jashkenas

This comment has been minimized.

Copy link
Contributor

jashkenas commented Jan 6, 2020

This issue was fixed by @BridgeAR in March 2019 but that change has yet to make it into a release. jashkenas/underscore@5304f86

I was pinged by @BridgeAR today, and published the updated test case as part of version 1.9.2.

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Jan 9, 2020

@nodejs/tsc PTAL. This could use some reviews.

}, `'${name}' is deprecated, use 'global'`, 'DEP0016');
}

ObjectDefineProperties(global, {

This comment has been minimized.

Copy link
@Trott

Trott Jan 10, 2020

Member

With this line removed, the file fails linting because ObjectDefineProperties is defined but never used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.