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.

Sign up for GitHub

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

Jump to bottom

module: move unnecessary work for early return #3579

Closed
wants to merge 1 commit into from
Closed

module: move unnecessary work for early return #3579

wants to merge 1 commit into from

Conversation

zertosh
Copy link
Contributor

@zertosh zertosh commented Oct 29, 2015

exts and trailingSlash are only used if the path isn't cached.

@Trott
Copy link
Member

Trott commented Oct 29, 2015

CI: https://ci.nodejs.org/job/node-test-pull-request/645/

@Trott
Copy link
Member

Trott commented Oct 29, 2015

The first line of the commit message should start with lib: rather than module:. No big deal if you don't fix it as whoever lands this can fix it. But if you can change it, that will save a few keystrokes for whoever lands it.

@zertosh zertosh changed the title module: move unnecessary work for early return lib: move unnecessary work for early return Oct 29, 2015
@zertosh
Copy link
Contributor Author

zertosh commented Oct 29, 2015

@Trott done!

@mscdex
Copy link
Contributor

mscdex commented Oct 29, 2015

@Trott Why should it start with lib: instead of module:? It seems like the latter would be a more descriptive subsystem target?

@Trott
Copy link
Member

Trott commented Oct 29, 2015

@mscdex I always thought (wrongly, it seems!) that we standardize on the nice short directory names in the top-level directory as much as possible. So lib, src, test, etc. module seemed weirdly long and specific to me . But looking through the commit log, there's plenty of precedence for it. So I retract!

@zertosh Sorry for the bad suggestion. lib: is fine but module: is better.

@zertosh zertosh changed the title lib: move unnecessary work for early return module: move unnecessary work for early return Oct 29, 2015
@zertosh
Copy link
Contributor Author

zertosh commented Oct 29, 2015

no worries @Trott

@Trott Trott added the module Issues and PRs related to the module subsystem. label Oct 29, 2015
@jbergstroem
Copy link
Member

Afaik -- prefixes usually stick to the filenames in lib/ if that's where most of the change is made.

@jasnell
Copy link
Member

jasnell commented Oct 29, 2015

@nodejs/ctc

@trevnorris
Copy link
Contributor

Mostly what @jbergstroem. The name should be the most closely related subsystem you pass to either require() or process.binding(). There are a few exceptions like src: (for general cleanup).

@rvagg
Copy link
Member

rvagg commented Oct 30, 2015

If it's not obvious, just pick one, module:, src: or node: will do, it only really matter where you're doing work specific to a subsystem or are clearly inside a subsystem, otherwise the subsystem prefix is almost useless so it's just a matter of coming up with an arbitrary grouping. Let's not bikeshed this ..

@zertosh
Copy link
Contributor Author

zertosh commented Oct 31, 2015

Seems like everyone is Ok on this- even the commit prefix. Merge?

EDIT: Meant this for #3578

@trevnorris
Copy link
Contributor

LGTM.

@jasnell
Copy link
Member

jasnell commented Nov 5, 2015

LGTM

@MylesBorins
Copy link
Member

@jasnell @trevnorris should this be merged?

@cjihrig
Copy link
Contributor

cjihrig commented Jan 7, 2016

LGTM. We could probably change the vars to const or let. And, we should get a fresh CI run.

Fishrock123
Fishrock123 reviewed Jan 7, 2016
View reviewed changes
lib/module.js
var cacheKey = JSON.stringify({request: request, paths: paths});
if (Module._pathCache[cacheKey]) {
return Module._pathCache[cacheKey];
}

var exts = Object.keys(Module._extensions);
var trailingSlash = (request.slice(-1) === '/');
Copy link
Member

Choose a reason for hiding this comment

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

While you're at it, the parenthesis on this line are unnecessary.

@jasnell
Copy link
Member

jasnell commented Jan 7, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/1162/

@zertosh
module: move unnecessary work for early return
be4744a 
`exts` and `trailingSlash` are only used if the path isn't cached.
@zertosh
Copy link
Contributor Author

zertosh commented Jan 8, 2016

@cjihrig: I've rebased on top of master and switched the var to const.

@cjihrig
Copy link
Contributor

cjihrig commented Jan 8, 2016

Thanks. Last CI was all green, but running one more time just to make sure switching to const didn't break anything. https://ci.nodejs.org/job/node-test-pull-request/1164/

@Trott
Copy link
Member

Trott commented Jan 8, 2016

Ci is green green green.

@targos
Copy link
Member

targos commented Jan 8, 2016

LGTM

cjihrig pushed a commit that referenced this pull request Jan 11, 2016
@zertosh @cjihrig
module: move unnecessary work for early return
1285671 
The exts and trailingSlash variables are only used if the
path isn't cached. This commit moves them further down in the
code, and changes from var to const.

PR-URL: #3579
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
@cjihrig
Copy link
Contributor

cjihrig commented Jan 11, 2016

Landed in 1285671. Thanks!

@cjihrig cjihrig closed this Jan 11, 2016
MylesBorins pushed a commit that referenced this pull request Jan 11, 2016
@zertosh
module: move unnecessary work for early return
9255d92 
The exts and trailingSlash variables are only used if the
path isn't cached. This commit moves them further down in the
code, and changes from var to const.

PR-URL: #3579
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 11, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Jan 11, 2016
2016-01-11, Version 5.4.1 (Stable)
002b9f8 
Notable Changes:

* Minor performance improvements:
  - lib: Use arrow functions instead of bind where possible (Minwoo Jung) #3622.
    - (Mistakenly missing from v5.4.0)
  - module: move unnecessary work for early return (Andres Suarez) #3579
* Various doc fixes
* Various test improvements

PR-URL: #4626
MylesBorins pushed a commit that referenced this pull request Jan 12, 2016
@zertosh
module: move unnecessary work for early return
145b668 
The exts and trailingSlash variables are only used if the
path isn't cached. This commit moves them further down in the
code, and changes from var to const.

PR-URL: #3579
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 12, 2016
2016-01-12, Version 5.4.1 (Stable)
ff99203 
Notable Changes:

* Minor performance improvements:
  - module: move unnecessary work for early return (Andres Suarez) #3579
* Various bug fixes
* Various doc fixes
* Various test improvements

PR-URL: #4626
MylesBorins pushed a commit that referenced this pull request Jan 13, 2016
2016-01-12, Version 5.4.1 (Stable)
a254507 
Notable Changes:

* Minor performance improvements:
  - module: move unnecessary work for early return (Andres Suarez) #3579
* Various bug fixes
* Various doc fixes
* Various test improvements

PR-URL: #4626
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 28, 2016
@zertosh
module: move unnecessary work for early return
94af231 
The exts and trailingSlash variables are only used if the
path isn't cached. This commit moves them further down in the
code, and changes from var to const.

PR-URL: #3579
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
@zertosh
module: move unnecessary work for early return
6499012 
The exts and trailingSlash variables are only used if the
path isn't cached. This commit moves them further down in the
code, and changes from var to const.

PR-URL: #3579
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 11, 2016
@zertosh
module: move unnecessary work for early return
8851920 
The exts and trailingSlash variables are only used if the
path isn't cached. This commit moves them further down in the
code, and changes from var to const.

PR-URL: nodejs#3579
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 11, 2016
Merged
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 15, 2016
@zertosh
module: move unnecessary work for early return
2e92a1a 
The exts and trailingSlash variables are only used if the
path isn't cached. This commit moves them further down in the
code, and changes from var to const.

PR-URL: nodejs#3579
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@zertosh
module: move unnecessary work for early return
77dceb4 
The exts and trailingSlash variables are only used if the
path isn't cached. This commit moves them further down in the
code, and changes from var to const.

PR-URL: nodejs#3579
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
2016-01-12, Version 5.4.1 (Stable)
253ebd5 
Notable Changes:

* Minor performance improvements:
  - module: move unnecessary work for early return (Andres Suarez) nodejs#3579
* Various bug fixes
* Various doc fixes
* Various test improvements

PR-URL: nodejs#4626
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
module Issues and PRs related to the module subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet