lib: remove broken NODE_MODULE_CONTEXTS feature #1162
Conversation
bnoordhuis
added
module
|
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 <[email protected]>
bnoordhuis
force-pushed
the
remove-node-module-contexts
branch
from
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. |
Fishrock123
removed
the
semver-minor
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/