[ui5-tooling-modules] Third-party dependency is skipped though it is listed in package.json as dependency

[AnsgarLichter]

Describe the bug
I have a UI5 application with a third-party dependency and a UI5 library. In our current setup, we require the library and the third-party dependency ajv as a dependency in the package.json.
As soon as I integrated the library, the deployed application stopped working as the third-party dependency was not longer included in the build output - the following warning was shown in the logs of the build process:
`Skipping dependency "ajv" as it is not part of the dependencies in package.json!``

The dependencies in the package.json of the application looks like the following:

"dependencies": {
    "<namespace>.reuse": "file:../reuse-lib"
    "ajv": "^8.17.1",
    "ajv-formats": "^3.0.1",
    "ajv-i18n": "^4.2.0"
 }

Currently, the library itself does not have any dependencies.

Additionally, there was an issue in the past which required us to add ajv as an additional dependency in the ui5.yaml of the application itself:

configuration:
        addToNamespace: true
        additionalDependencies: ["ajv"]

During the dependency resolution process in ui5-tooling-modules, the module looks for ajv in the node_modules directory of the library. This happens because ajv is also a dependency of eslint, which is used as a devDependency within the library. Consequently, ajv becomes a transitive dependency of the library through eslint, and the resolution process identifies it. As a result, ajv is added to depRoots, associating it with the library's path.

if (depRoot && depRoots.indexOf(depRoot) === -1) {
  depRoots.push(depRoot);
  depRoots.push(...findDependencies({ cwd: depRoot, depPaths, linkedOnly, additionalDeps }, knownDeps));
}

Later, ajv is also found as a dependency in the node_modules of the application. As this is already a known dependency, the path of the dependency is not added to the dependency roots:

if (knownDeps.indexOf(npmPackage) !== -1) {
  continue;
}

This leads to allowedDep being set to false during dependency resolution in the function addDep:

let allowedDep = existsDep && (!dependencyRoots || dependencyRoots.some((root) => depPath.startsWith(root)));

Therefore, the modules logs the warning above. For my use case, the issue is solved by removing the flag additionalDependencies. As I think that the same dependency could be required by a library and a application in a similar setup, I raise this issue so others can have a better experience setting this up.

Imo, it is necessary to push the third-party dependency to the dependencyRoots for all found occurrences, not only when it is an unkown dependency (not included in the knownDep arrray). I am sure that I only have touched the surface of the dependency resolution mechanism and there are much more side effects to consider.

To Reproduce
If required, I can provide a sample internally.

Expected behavior
The dependency resolution should recognize the dependecy correctly so that the build artefact is working and able to load the dependency during runtime.

Screenshots
N/A

Desktop (please complete the following information):

• OS: macOS
• Browser: Chrome
• Version: 3.20.2 (of ui5-tooling-modules)

Additional context
N/A

[petermuessig]

@AnsgarLichter thanks for the detailed analysis - this is indeed an issue I didn't have on the radar yet, that the transitive dependencies are found before the direct dependencies. I need to take a look into this... THX for the hint!

[AnsgarLichter]

@petermuessig That's great to hear. One more thing to add: The transitive dependencies are only found before the direct dependencies if the library is listed before the direct dependencies in the package.json. This means if the library is listed as the last dependency, then this behavior does not occur but it is not very transparent.

[petermuessig]

@AnsgarLichter I'm changing the lookup strategy now, which is IMO nevertheless better as this will only follow the really relevant transitive dependencies - I delay the recursive findDependencies call now - fix will come a bit later as I need to implement another feature in parallel - but this is how I intend to change the lookup logic:

	const depRoots = [];
	const findDeps = [];
	for (const dep of dependencies) {
		const npmPackage = npmPackageScopeRegEx.exec(dep)?.[1];
		if (knownDeps.indexOf(npmPackage) !== -1) {
			continue;
		}
		knownDeps.push(npmPackage);
		const depPath = findDependency(path.posix.join(npmPackage, "package.json"), cwd, depPaths);
		let depRoot = depPath && path.dirname(depPath);
		if (depRoot && depRoots.indexOf(depRoot) === -1) {
			depRoots.push(depRoot);
			// delay the dependency lookup to avoid finding transitive dependencies before local dependencies
			findDeps.push({ cwd: depRoot, depPaths, linkedOnly, additionalDeps });
		}
	}
	// lookup the transitive dependencies
	for (const dep of findDeps) {
		depRoots.push(...findDependencies(dep, knownDeps));
	}

[AnsgarLichter]

Without doing any further tests, this looks good to me and should solve the issue.
Thanks!