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

Revert "module: allow require('.')" #1358

Closed
wants to merge 1 commit into from
Closed

Revert "module: allow require('.')" #1358

wants to merge 1 commit into from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Apr 7, 2015

This reverts commit 6fc5e95. Closes #1356

@rvagg
Copy link
Member

rvagg commented Apr 7, 2015

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.

@brendanashworth brendanashworth added the module Issues and PRs related to the module subsystem. label Apr 7, 2015
@petkaantonov
Copy link
Contributor

I personally disagree with both revert and workaround, the bug used as feature is bizarre and easily fixable for the user who abused it.

@Fishrock123
Copy link
Contributor

I'd rather just make NODE_PATH work as expected with it, i.e. require the specified directory (Ignoring more than one because that doesn't even make sense).

@rlidwka
Copy link
Contributor

rlidwka commented Apr 7, 2015

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).

@chrisdickinson
Copy link
Contributor

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 require('.') work as folk expected. It seems like it would be way more surprising for a much larger group of people to add-and-then-remove this feature in patch or minor versions.

@brendanashworth
Copy link
Contributor

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 module module.

@bnoordhuis
Copy link
Member

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.

@silverwind
Copy link
Contributor

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.

@domenic
Copy link
Contributor

domenic commented Apr 8, 2015

If this revert lands it should cause a major bump. I am already using require('.') in shipping programs.

@silverwind
Copy link
Contributor

@domenic brings up a good point. reverting might cause more damage then keeping 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.

10 participants