Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed 'configObject' initilization statement to call 'requireFn()' … #104

Merged
merged 2 commits into from
Apr 13, 2016
Merged

Changed 'configObject' initilization statement to call 'requireFn()' … #104

merged 2 commits into from
Apr 13, 2016

Conversation

ec-milan
Copy link
Contributor

@ec-milan ec-milan commented Apr 9, 2016

Changed 'configObject' initilization statement to call 'requireFn()' function (instead of 'require()') when the 'config' option is provided as a string.

I have noticed that my Gulp plugins are not loaded when I provide the 'config' option. I have set the option to "package.json" (which is default) and it resolved to some other (undetermined) 'package.json' file. When 'config' option is not provided it resolves correctly since 'config' is then initialized by result of calling 'findup()' - which returns the full path to 'package.json' file inside the project folder (the one I wanted to target). It seems that resolving the filepath and setting the 'requireFn' is the goal of 'else if' block before the 'configObject' initialization, so it makes sense to utilize 'requireFn()' function instead of 'require()'.

…function (instead of 'require()') when the 'config' option is provided as a string.
@jackfranklin
Copy link
Owner

Great spot, looks good to me. Thank you!

You can also add yourself to the package.json as a contributor, and then I'll merge. Thanks so much :)

@ec-milan
Copy link
Contributor Author

It was a pleasure. Tnx!

@jackfranklin jackfranklin merged commit 420bd67 into jackfranklin:master Apr 13, 2016
@jackfranklin
Copy link
Owner

I'll release a new version today with this change!

@jackfranklin
Copy link
Owner

Out as 1.2.1, thanks again!

@mririgoyen
Copy link

This version change broke all of our current builds. We had to hard-code back 1.2.0. A minor-version increment should not be a breaking change. Hopefully this trace will help you resolve the issue:

/code/node_modules/gulp-load-plugins/node_modules/resolve/lib/sync.js:33
throw new Error("Cannot find module '" + x + "' from '" + y + "'");
^

Error: Cannot find module '../../package.json' from '../..'
at Function.module.exports as sync
at requireFn (/code/node_modules/gulp-load-plugins/index.js:46:25)
at module.exports (/code/node_modules/gulp-load-plugins/index.js:53:49)
at Object. (/code/services/app/gulpfile.js:6:37)
at Module._compile (module.js:409:26)
at Object.Module._extensions..js (module.js:416:10)
at Module.load (module.js:343:32)
at Function.Module._load (module.js:300:12)
at Module.require (module.js:353:17)
at require (internal/module.js:12:17)
at Liftoff.handleArguments (/usr/lib/node_modules/gulp/bin/gulp.js:116:3)
at Liftoff. (/usr/lib/node_modules/gulp/node_modules/liftoff/index.js:193:16)
at module.exports (/usr/lib/node_modules/gulp/node_modules/liftoff/node_modules/flagged-respawn/index.js:17:3)
at Liftoff. (/usr/lib/node_modules/gulp/node_modules/liftoff/index.js:185:9)
at /usr/lib/node_modules/gulp/node_modules/liftoff/index.js:159:9
at /usr/lib/node_modules/gulp/node_modules/v8flags/index.js:91:14
at FSReqWrap.oncomplete (fs.js:82:15)

Additionally, the plugin is initiated like so:

var $ = require('gulp-load-plugins')({
  config: '../../package.json'
});

@jackfranklin
Copy link
Owner

@goyney I apologise, I was under the impression that this wasn't a breaking change, else I wouldn't have released it as such. If you're able to push a simplified version that reproduces the bug that would be really useful. For now sticking to 1.2 shouldn't be that big of a deal.

@jackfranklin
Copy link
Owner

Alternatively you could pass requireFn: require as an option.

If anyone can provide me with a repo that reproduces that would help a lot.

@jackfranklin
Copy link
Owner

On second thoughts I don't actually think that this PR should have been merged; requireFn is designed to load the gulp plugins from the right location based on the location of the config, which means it shouldn't be used to load the config file itself. I'm going to revert this and release 1.2.2.

@mwessner open another issue if you're having problems and we can look into it.

@ec-milan
Copy link
Contributor Author

I've opened a new issue, please refer to #106 for further discussion...

@ec-milan ec-milan deleted the 'configObject'-initialization branch April 14, 2016 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants