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

module: fix require('.') #1185

Closed
wants to merge 1 commit into from
Closed

module: fix require('.') #1185

wants to merge 1 commit into from

Conversation

targos
Copy link
Member

@targos targos commented Mar 18, 2015

fixes #1178

@yosuke-furukawa
Copy link
Member

@chrisdickinson
Copy link
Contributor

LGTM.

While modules are locked, this seems like a bug fix, not a drastic change in existing behavior. It's exceedingly unlikely that anyone is depending on being able to require a file named ..

@Fishrock123 Fishrock123 added the module Issues and PRs related to the module subsystem. label Mar 19, 2015
@brendanashworth
Copy link
Contributor

Would this be semver-minor or semver-patch? It looks like a new feature to me.

@rvagg
Copy link
Member

rvagg commented Mar 19, 2015

good question, I'd probably lean toward semver-minor since we've become so used to require('./') that this is allowing new behaviour

@chrisdickinson
Copy link
Contributor

I'd say semver-patch, since it's a bugfix. It doesn't add a new API or any new docs, it just makes an existing API deliver perform closer to user expectation – I'd venture that no user would reasonably expect require('.') to attempt to load a file named ..js, given our documentation.

@targos
Copy link
Member Author

targos commented Mar 19, 2015

I will add a test tomorrow

@silverwind
Copy link
Contributor

I too think a patch is fine.

@targos targos force-pushed the fix-require branch 2 times, most recently from dec5944 to 6b4cd81 Compare March 20, 2015 13:44
@targos
Copy link
Member Author

targos commented Mar 20, 2015

test added

@tellnes
Copy link
Contributor

tellnes commented Mar 20, 2015

LGTM

@@ -205,7 +205,7 @@ Module._resolveLookupPaths = function(request, parent) {
}

var start = request.substring(0, 2);
if (start !== './' && start !== '..') {
if (request !== '.' && start !== './' && start !== '..') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

request --> start ? :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why this change is necessary. It could also have been written like this to eliminate the substring call:

if (request[0] !== '.' || (request[1] !== '/' && request[1] !== '.'))

But this isn't a hot path, so it does probably not matter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current one looks more readable to me, but feel free to follow up with a change if the perf difference is significant.

@silverwind
Copy link
Contributor

LGTM

I'll try a merge ;)

@silverwind silverwind self-assigned this Mar 20, 2015
silverwind pushed a commit that referenced this pull request Mar 20, 2015
Previously, the minimal argument to require the current directory was
require('./'). This commits allows to skip the trailing slash.

Fixes: #1178
PR-URL: #1185
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Christian Tellnes <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
@silverwind
Copy link
Contributor

Merged in 6fc5e95, thanks!

@silverwind silverwind closed this Mar 20, 2015
@ryanseys
Copy link
Contributor

awesome 👍

@targos targos deleted the fix-require branch March 21, 2015 12:36
@rvagg rvagg mentioned this pull request Mar 22, 2015
rvagg added a commit that referenced this pull request Mar 23, 2015
Notable Changes:

* Windows: The ongoing work in improving the state of Windows support
  has resulted in full test suite passes once again. As noted in the
  release notes for v1.4.2, CI system and configuration problems
  prevented it from properly reporting problems with the Windows
  tests, the problems with the CI and the codebase appear to have been
  fully resolved.
* FreeBSD: A kernel bug impacting io.js/Node.js was discovered and a
  patch has been introduced to prevent it causing problems for io.js
  (Fedor Indutny) #1218.
* module: you can now require('.') instead of having to require('./'),
  this is considered a bugfix (Michaël Zasso) #1185.
* v8: updated to 4.1.0.25 including patches for --max_old_space_size
  values above 4096 and Solaris support, both of which are already
  included in io.js.
@19h
Copy link
Contributor

19h commented Mar 25, 2015

Awesome! 👍

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