Skip to content

module: fix require('.') - #1185

Closed
targos wants to merge 1 commit into
nodejs:v1.xfrom
targos:fix-require
Closed

module: fix require('.')#1185
targos wants to merge 1 commit into
nodejs:v1.xfrom
targos:fix-require

Conversation

@targos

Copy link
Copy Markdown
Member

fixes#1178

@yosuke-furukawa

Copy link
Copy Markdown
Member

@chrisdickinson

Copy link
Copy Markdown
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 ..

@Fishrock123Fishrock123 added the module Issues and PRs related to the module subsystem. label Mar 19, 2015
@brendanashworth

Copy link
Copy Markdown
Contributor

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

@rvagg

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
MemberAuthor

I will add a test tomorrow

@silverwind

Copy link
Copy Markdown
Contributor

I too think a patch is fine.

@targos
targosforce-pushed the fix-require branch 2 times, most recently from dec5944 to 6b4cd81CompareMarch 20, 2015 13:44
@targos

Copy link
Copy Markdown
MemberAuthor

test added

@tellnes

Copy link
Copy Markdown
Contributor

LGTM

Comment threadlib/module.js Outdated

Copy link
Copy Markdown
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
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

done

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

LGTM

I'll try a merge ;)

@silverwindsilverwind 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 <christopher.s.dickinson@gmail.com>
Reviewed-By: Christian Tellnes <christian@tellnes.no>
Reviewed-By: Roman Reiss <me@silverwind.io>
@silverwind

Copy link
Copy Markdown
Contributor

Merged in 6fc5e95, thanks!

@ryanseys

Copy link
Copy Markdown
Contributor

awesome 👍

@targos
targos deleted the fix-require branch March 21, 2015 12:36
@rvaggrvagg 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

19h commented Mar 25, 2015

Copy link
Copy Markdown
Contributor

Awesome! 👍

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

moduleIssues and PRs related to the module subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@targos@yosuke-furukawa@chrisdickinson@brendanashworth@rvagg@silverwind@tellnes@ryanseys@19h@Fishrock123