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: remove broken NODE_MODULE_CONTEXTS feature #1162

Merged
merged 1 commit into from
Mar 16, 2015

Conversation

bnoordhuis
Copy link
Member

This feature has no tests and has been broken for ages, see for example
#1160. Don't bother fixing it, it's
pretty much broken by design and there can't be too many users because
it's almost undocumented. A quick Google search suggests that it causes
more grief than joy to the few that do use it. Remove it.

R=@iojs/tc

https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/306/

@bnoordhuis bnoordhuis added module Issues and PRs related to the module subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Mar 16, 2015
@cjihrig
Copy link
Contributor

cjihrig commented Mar 16, 2015

LGTM. CI looks good.

@vkurchatkin
Copy link
Contributor

why is this semver minor?

@bnoordhuis
Copy link
Member Author

@vkurchatkin As opposed to? Major or patch?

@vkurchatkin
Copy link
Contributor

@bnoordhuis looks like major to me

@bnoordhuis
Copy link
Member Author

@vkurchatkin I'd agree if it weren't broken, but it is and has been since at least 2013. I think that makes it eligible for quick removal, particularly when you consider that no one bothered to report it here or at joyent/node until now.

(Also, google for it - all the links are about people having issues with it.)

@chrisdickinson
Copy link
Contributor

@bnoordhuis If it was broken and no one uses it, I'd be okay with knocking this down to semver-patch.

@vkurchatkin
Copy link
Contributor

this is still a breaking change, isn't it? I have no idea why this feature exists but just removing it seems not right. Why not just deprecate it as usual?

@bnoordhuis
Copy link
Member Author

Because it's broken and evidently no one uses it. I'm all for keeping working code working but I don't think that's a factor here.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 16, 2015

+1 for patch. Can't be a breaking change if it's already broken.

This feature has no tests and has been broken for ages, see for example
nodejs#1160.  Don't bother fixing it, it's
pretty much broken by design and there can't be too many users because
it's almost undocumented.  A quick Google search suggests that it causes
more grief than joy to the few that do use it.  Remove it.

PR-URL: nodejs#1162
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@bnoordhuis bnoordhuis force-pushed the remove-node-module-contexts branch from 0708d4d to f58e596 Compare March 16, 2015 19:58
@bnoordhuis bnoordhuis closed this Mar 16, 2015
@bnoordhuis bnoordhuis deleted the remove-node-module-contexts branch March 16, 2015 19:58
@bnoordhuis bnoordhuis merged commit f58e596 into nodejs:v1.x Mar 16, 2015
@bnoordhuis
Copy link
Member Author

Thanks everyone. I've landed it in f58e596. If it turns out someone does have a non-broken use case for it (unlikely but hey), we'll just revert it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Issues and PRs related to the module subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants