module: move unnecessary work for early return #3579

zertosh · 2015-10-29T03:15:19Z

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

Trott · 2015-10-29T03:23:36Z

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

Trott · 2015-10-29T03:25:48Z

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 · 2015-10-29T03:28:09Z

@Trott done!

mscdex · 2015-10-29T03:49:45Z

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

Trott · 2015-10-29T03:56:33Z

@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 · 2015-10-29T03:58:29Z

no worries @Trott

jbergstroem · 2015-10-29T05:08:16Z

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

jasnell · 2015-10-29T18:57:54Z

@nodejs/ctc

trevnorris · 2015-10-29T21:07:06Z

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 · 2015-10-30T01:51:15Z

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 · 2015-10-31T23:32:39Z

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

EDIT: Meant this for #3578

trevnorris · 2015-11-01T15:43:07Z

LGTM.

jasnell · 2015-11-05T17:58:59Z

LGTM

MylesBorins · 2016-01-07T00:24:39Z

@jasnell @trevnorris should this be merged?

cjihrig · 2016-01-07T00:27:21Z

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

Fishrock123 · 2016-01-07T16:26:53Z

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) === '/');


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

jasnell · 2016-01-07T23:47:12Z

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

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

zertosh · 2016-01-08T02:47:22Z

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

cjihrig · 2016-01-08T03:02:17Z

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 · 2016-01-08T04:38:09Z

Ci is green green green.

targos · 2016-01-08T09:01:18Z

LGTM

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 <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

cjihrig · 2016-01-11T18:18:41Z

Landed in 1285671. Thanks!

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 <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

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

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 <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

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

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 <[email protected]>

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 <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

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 <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

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 <[email protected]>

zertosh changed the title ~~module: move unnecessary work for early return~~ lib: move unnecessary work for early return Oct 29, 2015

zertosh changed the title ~~lib: move unnecessary work for early return~~ module: move unnecessary work for early return Oct 29, 2015

Trott added the module Issues and PRs related to the module subsystem. label Oct 29, 2015

jasnell added the lts-watch-v4.x label Nov 5, 2015

Trott force-pushed the master branch from 1e896a6 to 082cc8d Compare December 27, 2015 02:00

Fishrock123 reviewed Jan 7, 2016
View reviewed changes

module: move unnecessary work for early return

be4744a

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

cjihrig closed this Jan 11, 2016

MylesBorins mentioned this pull request Jan 11, 2016

V5.4.1 propose #4626

Merged

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jan 28, 2016

MylesBorins mentioned this pull request Feb 11, 2016

V4.3.1 proposal #5200

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: move unnecessary work for early return #3579

module: move unnecessary work for early return #3579

zertosh commented Oct 29, 2015

Trott commented Oct 29, 2015

Trott commented Oct 29, 2015

zertosh commented Oct 29, 2015

mscdex commented Oct 29, 2015

Trott commented Oct 29, 2015

zertosh commented Oct 29, 2015

jbergstroem commented Oct 29, 2015

jasnell commented Oct 29, 2015

trevnorris commented Oct 29, 2015

rvagg commented Oct 30, 2015

zertosh commented Oct 31, 2015

trevnorris commented Nov 1, 2015

jasnell commented Nov 5, 2015

MylesBorins commented Jan 7, 2016

cjihrig commented Jan 7, 2016

Fishrock123 Jan 7, 2016

jasnell commented Jan 7, 2016

zertosh commented Jan 8, 2016

cjihrig commented Jan 8, 2016

Trott commented Jan 8, 2016

targos commented Jan 8, 2016

cjihrig commented Jan 11, 2016

module: move unnecessary work for early return #3579

module: move unnecessary work for early return #3579

Conversation

zertosh commented Oct 29, 2015

Trott commented Oct 29, 2015

Trott commented Oct 29, 2015

zertosh commented Oct 29, 2015

mscdex commented Oct 29, 2015

Trott commented Oct 29, 2015

zertosh commented Oct 29, 2015

jbergstroem commented Oct 29, 2015

jasnell commented Oct 29, 2015

trevnorris commented Oct 29, 2015

rvagg commented Oct 30, 2015

zertosh commented Oct 31, 2015

trevnorris commented Nov 1, 2015

jasnell commented Nov 5, 2015

MylesBorins commented Jan 7, 2016

cjihrig commented Jan 7, 2016

Fishrock123 Jan 7, 2016

Choose a reason for hiding this comment

jasnell commented Jan 7, 2016

zertosh commented Jan 8, 2016

cjihrig commented Jan 8, 2016

Trott commented Jan 8, 2016

targos commented Jan 8, 2016

cjihrig commented Jan 11, 2016