module: remove dead code

[BridgeAR]

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]

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

[targos]

Can you rebase ?

[nodejs-github-bot]

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

[BridgeAR]

@nodejs/modules @nodejs/tsc PTAL.

[nodejs-github-bot]

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

[BridgeAR]

Rebased due to conflicts.

[nodejs-github-bot]

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

[jkrems]

LGTM

[BridgeAR]

@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]

@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]

@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]

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

[MylesBorins]

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]

@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]

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]

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

[vmarchaud]

@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]

Thanks everyone for the reviews.

Landed in d11c4be 🎉

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