-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
Revert "module: allow require('.')" #1358
Conversation
This reverts commit 6fc5e95.
Can we have some more discussion on whether to workaround or revert please? I'm weary of reverting features we've just newly introduced, if we think they are a good idea then we should stick to that conviction. |
I personally disagree with both revert and workaround, the bug used as feature is bizarre and easily fixable for the user who abused it. |
I'd rather just make |
I think in the future this sort of changes should appear in minor versions. I know it's a bugfix, but it's not nice to break patch versions. But the patch is already there. I'm using it personally when 0.10 compatibility is not an issue. And reverting it now only to re-land it later doesn't really seem like a good option. Maybe detect it somehow and print a warning, so people could find what's wrong quickly (if anyone even used this quirk). |
I'm also loathe to revert this, though strict semver says we should (though, if we were really, really strict the revert would be a major version bump too!) #1356 seems like usage that sits at the cross section of some fairly arcane (and disused) features, whereas the patch in question made |
It may be noted that #1185 was merged in as semver-patch, so I don't think reverting a broken bug fix in strict semver is semver-major. I'll vote that a patch to fix the introduced bug would be preferable. Otherwise, I still say revert - but only because its the locked |
I'm surprised there is even any discussion. The patch broke someone's workflow and the proposed alternative was to add a heuristic that is at best half-baked, at worst a source of new regressions. In my opinion the only safe course is to revert now and land again at the next major bump. |
Don't revert just yet. I'll try fixing OP's case while still allowing to require('.') to work as expected. It will be a hack that should be removed asap, but I think its the best course of action here. |
If this revert lands it should cause a major bump. I am already using require('.') in shipping programs. |
@domenic brings up a good point. reverting might cause more damage then keeping it. |
This reverts commit 6fc5e95. Closes #1356