module: remove dead code #26983

BridgeAR · 2019-03-29T14:54:28Z

This removes a lot of code that has no functionality anymore. All
Node.js internal code calls _resolveLookupPaths with two arguments.

The code that validates index.js is not required at all as we check
for these files anyway, so it's just redundant code that should be
removed.

I defensively marked this as semver-major even though I did not find any usage of the third argument outside of Node.js and that would only be an issue if that part was monkey patched.

Refs: #25362

@nodejs/modules PTAL

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2019-03-29T14:54:31Z

@BridgeAR Sadly, an error occurred when I tried to trigger a build. :(

targos · 2019-03-30T13:58:08Z

Can you rebase ?

nodejs-github-bot · 2019-03-31T10:41:37Z

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

BridgeAR · 2019-04-01T14:59:54Z

@nodejs/modules @nodejs/tsc PTAL.

lib/internal/modules/cjs/loader.js

This removes a lot of code that has no functionality anymore. All Node.js internal code calls `_resolveLookupPaths` with two arguments. The code that validates `index.js` is not required at all as we check for these files anyway, so it's just redundant code that should be removed.

nodejs-github-bot · 2019-04-03T01:55:30Z

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

BridgeAR · 2019-04-03T01:55:53Z

Rebased due to conflicts.

nodejs-github-bot · 2019-04-03T12:16:42Z

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

hybrist

LGTM

BridgeAR · 2019-04-04T12:54:29Z

@nodejs/tsc I would actually like to land this as patch instead of semver-major. I just checked gzemnid and I could not find a single topcode module using the function with three arguments or it is set to true.
Update: I made a mistake here. This is definitely a breaking change due to the changed signature. Only a few actually used modules rely upon that and I'll open PR's to fix these.

@nodejs/modules PTAL

This is really quite a lot of dead code that is hit on each request and thus even minimally slowing down loading.

ljharb · 2019-04-04T15:45:47Z

@BridgeAR could you not remove the dead code in one backportable patch PR, without removing the argument, and land the breaking argument change separately?

BridgeAR · 2019-04-04T15:51:36Z

@ljharb I could separate that but the returned id would be changed in that case. I guess that property is not used outside of Node.js core and people monkey patching this function only rely upon the second array entry but that's difficult to tell for sure. That's why I wanted to land this together.

BridgeAR · 2019-04-05T18:47:53Z

@nodejs/tsc PTAL. This is semver-major and still requires reviews from the TSC.

MylesBorins · 2019-04-05T20:52:52Z

So a thought... will removing this stuff make it harder for us to backport some of the modules stuff to 10.x?

Would it maybe be better to do this change for 13 and keep the delta smaller between 10 and 12?

guybedford

Nice work.

@MylesBorins which commits are you thinking about specifically here? If this is about ES Modules, the diffs seem separate enough for that not to be an issue. If it is the other PRs done by @BridgeAR or on CommonJS, it would definitely make sense to ensure any CJS loader code is ported in the same order.

BridgeAR · 2019-04-05T21:07:28Z

@MylesBorins

will removing this stuff make it harder for us to backport some of the modules stuff to 10.x

This should not make that any more difficult. As @guybedford pointed out this diff is separate from any ES Modules code. I believe there where a few things that have to be backported as well for the ES Modules but I don't know about them exactly as I did not do those. AFAIK non of my changes touched any of these things.

Trott

LGTM if CITGM doesn't reveal any surprises

BridgeAR · 2019-04-06T17:37:00Z

CITGM looks good.

The following modules came up with https://gzemnid.nodejs.org:

899740	vxx-1.2.2.tgz/src/util.js:101:  var resolvedModule = Module._resolveLookupPaths(request, parent);
736731	requizzle-0.2.1.tgz/lib/requizzle.js:43:	var lookupPaths = Module._resolveLookupPaths(targetPath, parentModule);
47202	yui-3.18.1.tgz/get-nodejs/get-nodejs-debug.js:109:                url = Module._findPath(url, Module._resolveLookupPaths(url, module.parent.parent)[1]);
47202	yui-3.18.1.tgz/get-nodejs/get-nodejs.js:105:                url = Module._findPath(url, Module._resolveLookupPaths(url, module.parent.parent)[1]);
47202	yui-3.18.1.tgz/yui-nodejs/yui-nodejs-debug.js:4386:                url = Module._findPath(url, Module._resolveLookupPaths(url, module.parent.parent)[1]);
47202	yui-3.18.1.tgz/yui-nodejs/yui-nodejs.js:4135:                url = Module._findPath(url, Module._resolveLookupPaths(url, module.parent.parent)[1]);
45159	babel-plugin-recharts-1.2.0.tgz/lib/index.js:51:  var _Module$_resolveLooku = _module2.default._resolveLookupPaths(source, finalModule),
4998	rigger-1.0.0.tgz/index.js:305:                .concat(mod._resolveLookupPaths(this.cwd)[1]);

Update: I removed modules that were not impacted from the list.

vxx: redirects to @pm2/legacy-tracing. Fix open chore: update internal Node.js function call keymetrics/trassingue#4.
requizzle: Fix open Future proof hegemonic/requizzle#4. It does not seem to be maintained anymore (last commit is from 2015). It is mainly used for JSDoc.
yui: Fix open Update internal Node.js function call yui/yui3#2016. Module is not maintained since 2014.
babel-plugin-recharts: Can't determine it is impacted or not but I can't fix it either since it has no corresponding github repository.
rigger: Fix open Fix deprecation warning & remove Module._resolveLookupPaths usage buildjs/rigger#36. Module was last updated in 2017.

BridgeAR · 2019-04-08T13:16:08Z

This already has a couple LGs but it needs one more LG from the @nodejs/tsc. PTAL

vmarchaud · 2019-04-08T15:13:14Z

@BridgeAR Thanks again for taking the time to make a PR to keymetrics/trassingue but we stopped using this code few months ago, i've archived the repo

BridgeAR · 2019-04-08T16:58:24Z

Thanks everyone for the reviews.

Landed in d11c4be 🎉

@vmarchaud thanks for letting me know. That was the most impacted module.

BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 29, 2019

BridgeAR force-pushed the remove-module-dead-code branch from 82ad1bd to 846196e Compare March 31, 2019 10:40

BridgeAR requested a review from targos March 31, 2019 10:45

BridgeAR requested a review from mcollina April 1, 2019 15:00

guybedford reviewed Apr 1, 2019

View reviewed changes

lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved

BridgeAR requested a review from guybedford April 3, 2019 00:29

BridgeAR force-pushed the remove-module-dead-code branch from 846196e to 1abfc93 Compare April 3, 2019 01:54

BridgeAR requested review from devsnek, hybrist and jasnell April 3, 2019 12:15

hybrist approved these changes Apr 3, 2019

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 3, 2019

jasnell approved these changes Apr 4, 2019

View reviewed changes

guybedford approved these changes Apr 5, 2019

View reviewed changes

BridgeAR mentioned this pull request Apr 5, 2019

chore: update internal Node.js function call keymetrics/trassingue#4

Closed

BridgeAR mentioned this pull request Apr 5, 2019

Update internal Node.js function call yui/yui3#2016

Open

Trott approved these changes Apr 6, 2019

View reviewed changes

BridgeAR mentioned this pull request Apr 7, 2019

Fix deprecation warning & remove Module._resolveLookupPaths usage buildjs/rigger#36

Merged

targos approved these changes Apr 8, 2019

View reviewed changes

BridgeAR closed this Apr 8, 2019

This was referenced Apr 11, 2019

Node.js 12 SemVer Major Cut-off #26844

Closed

v12.0.0 proposal #26930

Merged

BridgeAR deleted the remove-module-dead-code branch January 20, 2020 11:59

Uh oh!

module: remove dead code #26983

module: remove dead code #26983

Uh oh!

Conversation

BridgeAR commented Mar 29, 2019

Checklist

Uh oh!

nodejs-github-bot commented Mar 29, 2019

Uh oh!

targos commented Mar 30, 2019

Uh oh!

nodejs-github-bot commented Mar 31, 2019

Uh oh!

BridgeAR commented Apr 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 3, 2019

Uh oh!

BridgeAR commented Apr 3, 2019

Uh oh!

nodejs-github-bot commented Apr 3, 2019

Uh oh!

hybrist left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Apr 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljharb commented Apr 4, 2019

Uh oh!

BridgeAR commented Apr 4, 2019

Uh oh!

BridgeAR commented Apr 5, 2019

Uh oh!

MylesBorins commented Apr 5, 2019

Uh oh!

guybedford left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Apr 5, 2019

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Apr 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Apr 8, 2019

Uh oh!

vmarchaud commented Apr 8, 2019

Uh oh!

BridgeAR commented Apr 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

BridgeAR commented Apr 1, 2019 •

edited

Loading

BridgeAR commented Apr 4, 2019 •

edited

Loading

BridgeAR commented Apr 6, 2019 •

edited

Loading