browserify / browserify/module-deps

_isTopLevel() check for "node_modules" transform exclusion is too "loose", should be more specific

Open
#132 3 comments 0 reactions 0 assignees View on GitHub
Dominant language
JavaScript
Stars
208
Forks
102
PR merge metrics
No merged PRs in 30d

Description

module-deps currently contains logic that results in excluding files from transformation if any segment in the file's package-relative path is the string "node_modules". This logic lives within the ```_isTopLevel(file)``` function, which determines if the specified file should be considered "top-level" (meaning, a file belonging to the package's own source code). Elsewhere in module-deps, only files considered top-level are eligible for transformation.

But, the current implementation of this logic appears to be far too "loose", in that it can end up excluding files from transformation that _should_ indeed be transformed!

Here is the problem...

In some projects, internal "node_modules" folders are used simply to facilitate use of the Node Module Resolution Algorithm (NMRA) locally within the package's own source code. Of course, depending on the needs of the project, any or all of this source code may require some kind of transformation.

Consider this example package file structure which uses an internal "node_modules" folder:
```
package.json
/src
main.js
/node_modules
foo.js
bar.js
/node_modules
(npm-installed packages)
```
Here, the ```/src/node_modules``` folder is intended to facilitate the use of NMRA between ```main.js```, ```foo.js```, and ```bar.js```. For example, with this organization, both ```main.js``` and ```bar.js``` can access ```foo.js``` via ```require("foo")```.

To be clear, the content of the ```/src/node_modules``` folder is NOT managed by npm. This particular "node_modules" folder has just been strategically placed to enable use of NMRA internally between the files under ```/src```.

The above is a very simple, contrived example, but in larger, actual projects, such internal use of NMRA can eliminate "require path hell" and aid modular design.

Anyway... it appears to me that the logic in ```_isTopLevel()``` is likely intended to assume that anything "outside" of the _root_ ```/node_modules``` (which normally contains already-built packages that should not be transformed) is a "top-level" file. Normally, top-level files end up being anything in ```/src``` or some such location.

The fly in the ointment, though, is that ```_isTopLevel()``` doesn't just check for "node_modules" specifically in as the 0th element of the candidate file's path... it checks for "node_modules" _anywhere_ in the path. Thus, ```_isTopLevel()``` erroneously concludes that files like ```/src/node_modules/foo.js``` are NOT top-level, and are thus excluded from transformation.

IMO, ```_isTopLevel()``` really should only check to see if a file lives below ```/node_modules``` _specifically_. Files everywhere else should be considered top-level, and eligible for transformation.

The proposed change in logic is this:

```javascript
Deps.prototype._isTopLevel = function (file) {
var isTopLevel = this.entries.some(function (main) {
var m = relativePath(path.dirname(main), file);
// EXISTING:
// return m.split(/[\\\/]/).indexOf('node_modules') < 0;
// PROPOSED:
return m.split(/[\\\/]/).indexOf('node_modules') !== 0;
});
if (!isTopLevel) {
var m = relativePath(this.basedir, file);
// EXISTING:
// isTopLevel = m.split(/[\\\/]/).indexOf('node_modules') < 0;
// PROPOSED:
isTopLevel = m.split(/[\\\/]/).indexOf('node_modules') !== 0;
}
return isTopLevel;
};
```

That is, "tightly" check for "node_modules" appearing only as the 0th element of the path (as it will for any file within ```/node_modules/**```), rather than "loosely" triggering off "node_modules" appearing anywhere in the path (which, as shown in the above example, probably doesn't mean what ```_isTopLevel()``` thinks it means).

Alternatively, it would be an improvement if the above "loose" assumption was simply not "baked into" module-deps, and instead was configurable (for those projects that need it).

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.