Resolving manually provided config file

[ec-milan]

Hi! This is a follow-up on previous PR/issue on resolving manually provided config file. Let's try to get to the bottom of this...

The problem I initially faced happens in the following scenario:

OS: Windows 10 Pro.
Shell: PowerShell 5.0.10586.122

Here is the relevant file structure on the filesystem:

/
|__ node_modules/
|   |
|   |__ ...
|
|__ testing/
|   |
|   |__ src/
|   |   |__ script1.js
|   |   |__ script2.js
|   |
|   |__ app/
|
|__ test_package.json
|__ gulpfile.js
|
|__ ...

Here is the content of simplified 'test_package.json' file:

{
  "name": "testcase",
  "version": "1.0.0",
  "devDependencies": {
    "gulp": "^3.9.1",
    "gulp-load-plugins": "latest",
    "gulp-concat": "^2.6.0"
  }
}

Here is the content of simplified 'testing/gulpfile.js' file:

var gulp = require("gulp");
var gulpLoadPlugins = require("gulp-load-plugins");

var loadPluginsOptions = {
    "config": "./test_package.json"
};

var plugins = gulpLoadPlugins(loadPluginsOptions);

gulp.task("default", function () {
    gulp.src("./testing/src/*.js")
        .pipe(plugins.concat("all.js"))
        .pipe(gulp.dest("./testing/app/"));
});

When I try to run the Gulp script from within the root directory I get the 'Cannot find module...' error, like so:

Changing the 'configObject' declaration solved the particular issue, but it was not a valid solution! I did not go deep enough into the problem... The case is: if our config file is named 'package.json' (which is the default value) but we do provide config path as './package.json' (which should be legit) - it resolves to 'package.json' of gulp-load-modules package:

and it consequently results in an error shown bellow:

I guess, this behavior is the result of search path starting from CWD. If our config is defined in a file named anything other than 'package.json', the error shown first above arises (in my case the config file was named 'test_package.json').

So, here is another simple test case (as noticed by @goyney) which highlights the real problem:

Here is the relevant file structure on the filesystem:

/
|__ node_modules/
|   |
|   |__ ...
|
|__ testing/
|   |
|   |__ src/
|   |   |__ script1.js
|   |   |__ script2.js
|   |
|   |__ app/
|   |
|   |__ gulpfile.js   // Changed from the first example!
|
|__ test_package.json
|
|__ ...

Content of 'test_package.json' file remains the same.

Here is the simplified content of 'testing/gulpfile.js' file:

var gulp = require("gulp");
var gulpLoadPlugins = require("gulp-load-plugins");

var loadPluginsOptions = {
    "config": "./../test_package.json"   // Changed from the first example!
};

var plugins = gulpLoadPlugins(loadPluginsOptions);

gulp.task("default", function () {
    gulp.src("./testing/src/*.js")
        .pipe(plugins.concat("all.js"))
        .pipe(gulp.dest("./testing/app/"));
});

When I try to run the Gulp script from within 'testing' directory I get the 'Cannot find module...' error, like so:

And when I change the 'configObject' declaration to call 'configureFn' I get the following error:

After putting some more effort to it I realized it was a problem of config file path, not the invoking function. So, here is proposed solution - adding the following block of code after options initialization:

// If config file path is provided as string value it should be normalized
// and converted to fixed path in order to resolve the file correctly on
// all platforms (Win, *nix, OSX, ...).
if (typeof config === 'string') {
    config = path.normalize(config);
    config = path.resolve(config);
}

This solves it for me - 'configObject' declaration stays the same ('require()' is used), but it resolves correctly for both './configfile' and './../configfile'.

Sidenote: It is hard for me to believe this problem did not surface earlier. I was testing this on Windows platform only, so it might be that on other platforms filepaths are resolved correctly even though no explicit 'path.normalize()' and 'path.resolve()' are utilized.

Can you please look into it...

P. S.
I am working on more than a few things at the moment, so I might not be able to respond promptly. :(

[mririgoyen]

test_package.json is an invalid filename though. The package.json filename is a built-in constant to node. Why would you expect that to work?

I personally do not feel a plugin should accommodate non-standard behavior. What is your reasoning for trying to change the name of the package file?

[ec-milan]

The problem I encountered is not caused by the file name - it is all about file path. So, even './../../package.json' will result in an error (at least on Windows). The solution I proposed takes care of both file path and file name while being platform-agnostic.

Regarding the file name per se - as I understand it: the whole idea of config option is to target the configuration file - whatever it may be named (as long as it respects the configuration format). Actually I never thought about why one might use different filename (or another file altogether) to handle Gulp modules, but that is completely irrelevant at this point. I ended up using different filename during debugging since it helped me avoid the behavior shown above in the second picture.

[jackfranklin]

If you do config: require('./test-package.json') does that work? I think it might, and if so I think documenting that that is something you can do might be a preferred approach here.

Thanks for writing this up though - I will try to take a more detailed look at it soon. If you could get the above examples into repos I could grab and work with that'd be really useful :)

[ec-milan]

@jackfranklin, providing the config object by doing config: require() works fine. So, it made me investigate further and finally I realized that path to the config file provided in gulpfile.js should be written relative to the index.js - ie. load-gulp-plugins directory (not relative to the gulpfile). On the other hand, when require() is invoked from within the gulpfile.js, the path of config file should be provided relative to the gulpfile itself. That makes sense, since require() searches relative to the process.cwd(). If no config option is provided, the config file is searched relative to the gulpfile due to the explicit findup() call. That is why config: "./package.json" resulted in plugin's own configuration file being read and all other mistakes I encountered. I did not find it obvious, although there is a comment in the Options section. The solution I proposed above would make it sure that config file is always searched relative to the gulpfile.
Feel free to close this issue - hopefully it will benefit someone in the future. I agree about documenting it - it would probably be helpful to highlight the usage of config option by adding a short explanation in Options section of the README file, e.g.:
Configuration can be provided:
(1) in the form of an object: config: require("./config.json") (file path relative to the gulpfile)
(2) in the form of a string: config: "./../../config.json" (file path relative to the gulp-load-plugin root directory).

Keep up the good work!

[jackfranklin]

@mwessner sorry I never replied to this comment - do you fancy making a PR with those documentation changes? That would be much appreciated if you have the time :)