-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
lib: remove broken NODE_MODULE_CONTEXTS feature #1162
Conversation
LGTM. CI looks good. |
why is this semver minor? |
@vkurchatkin As opposed to? Major or patch? |
@bnoordhuis looks like major to me |
@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.) |
@bnoordhuis If it was broken and no one uses it, I'd be okay with knocking this down to semver-patch. |
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? |
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. |
+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>
0708d4d
to
f58e596
Compare
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. |
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/