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

[node] Deprecate GLOBAL in favor of global #1881

Merged
merged 1 commit into from
Oct 7, 2016
Merged

[node] Deprecate GLOBAL in favor of global #1881

merged 1 commit into from
Oct 7, 2016

Conversation

sigv
Copy link
Contributor

@sigv sigv commented Oct 7, 2016

The GLOBAL variable should not be used in order to avoid DeprecationWarning: 'GLOBAL' is deprecated, use 'global'.

The `GLOBAL` variable should not be used in order to avoid
`DeprecationWarning: 'GLOBAL' is deprecated, use 'global'`.
@sigv
Copy link
Contributor Author

sigv commented Oct 7, 2016

This PR is a replacement for #1880.

@FooBarWidget
Copy link
Member

Thanks for the rebase. There is only one concern I have, and that is compatibility. You mentioned that the lowercase global was introduced in Node.js 6. We want to be compatible with at least the current LTS version so at the very least it needs to be compatible with Node.js 4. Maybe you can add some kind of if statement there to ensure compatibility?

And can I ask you to fill in our contributor agreement if you haven't filled it in already? We need it before can merge pull requests.

@sigv
Copy link
Contributor Author

sigv commented Oct 7, 2016

As I said in the old PR, this commit marks the GLOBAL being deprecated.

It was initially added back in the day with official documentation up until version 0.1.26.
In 0.1.27, the GLOBAL was renamed global. See Downcase process.ARGV, process.ENV, GLOBAL (now process.argv, process.env, global) in changelog and global instead of GLOBAL in documentation.

With that in mind, I do not see how this change can break compatibility even if you support versions down to 0.10.x. Even more so, in all other uses of the global object in the project, it is already used as global and not GLOBAL. (Example.)

@sigv
Copy link
Contributor Author

sigv commented Oct 7, 2016

Oh, and I have signed the agreement but I have not signed the commit. Is that okay for you guys?

@FooBarWidget
Copy link
Member

Ah I see, then I misunderstood. I thought you meant that in version 6 global was introduced and GLOBAL was deprecated, but global has existed for much longer. That's fine then.

@FooBarWidget FooBarWidget merged commit 31c4903 into phusion:stable-5.0 Oct 7, 2016
@FooBarWidget
Copy link
Member

It's been merged now, and I've added you to the CONTRIBUTORS file. Thanks again for the pull request!

@sigv sigv deleted the node-deprecate-global-5.0 branch October 7, 2016 20:05
@sigv
Copy link
Contributor Author

sigv commented Nov 7, 2016

What exactly is the release schedule of Passenger?
Is there some ETA for this to be released in an official open-source build of Passenger?

@OnixGH OnixGH added this to the 5.1.0 milestone Nov 7, 2016
@OnixGH
Copy link
Contributor

OnixGH commented Nov 7, 2016

It's merged to stable, so it's for the upcoming release. We typically release a few times per quarter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants