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

Throw if cwd option isn't a string #8

Merged
merged 4 commits into from Jan 10, 2019

Conversation

dead-horse
Copy link
Contributor

globby will pass opts.cwd = undefined to dir-glob, which leads to TypeError.

@dead-horse
Copy link
Contributor Author

ping @sindresorhus

@sindresorhus
Copy link
Contributor

I think this should be fixed in globby. The option handling here is correct.

@dead-horse
Copy link
Contributor Author

options.cwd could be undefined or null somehow, I think we should ensure dir-glob won't throw here?

@sindresorhus
Copy link
Contributor

IMHO, it's up to the caller to pass in the correct arguments. dir-glob should not handle invalid input.

@dead-horse
Copy link
Contributor Author

at least we should alert user opts.cwd set to undefined is invalid to avoid this TypeError which is hard to debug.

@sindresorhus
Copy link
Contributor

Agreed

@dead-horse
Copy link
Contributor Author

you'll fix this in globby and I change this PR to alert this invalid input?

@doochik
Copy link

doochik commented Jan 9, 2019

I've created PR to globby sindresorhus/globby#99

@hashar
Copy link

hashar commented Jan 9, 2019

See also:

#10
sindresorhus/globby#98

:)

@dead-horse dead-horse changed the title fix: make opts.cwd to process.cwd() when opts.cwd is undefined fix: throw TypeError if opts.cwd is not a string Jan 9, 2019
@@ -24,9 +24,11 @@ const getGlob = (dir, opts) => {

if (opts.files && opts.extensions) {
return opts.files.map(x => path.join(dir, addExtensions(x, opts.extensions)));
} else if (opts.files) {
}
if (opts.files) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes are unrelated.

package.json Outdated
@@ -29,10 +29,10 @@
"path-type": "^3.0.0"
},
"devDependencies": {
"ava": "*",
"ava": "~0.25.0",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also unrelated.

index.js Outdated
@@ -43,6 +48,9 @@ module.exports = (input, opts) => {

module.exports.sync = (input, opts) => {
opts = Object.assign({cwd: process.cwd()}, opts);
if (typeof opts.cwd !== 'string') {
throw new TypeError('opts.cwd must be a string');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new TypeError(`Expected `cwd` to be of type \`string\` but received type \`${typeof opts.cwd}\``);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error message updated, all the other changes made the project pass the lint by xo, pin ava and xo to ensure it work in node v4.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've make these changes into two commit if you want to separate them.

@kevva kevva changed the title fix: throw TypeError if opts.cwd is not a string Throw if cwd option isn't a string Jan 9, 2019
@kevva kevva merged commit 3cd414f into kevva:master Jan 10, 2019
@kevva
Copy link
Owner

kevva commented Jan 10, 2019

Thanks @dead-horse!

robcresswell added a commit to snyk/cli that referenced this pull request Jan 10, 2019
The CLI build process is currently broken due to an interaction between
pkg's "globby" dependency and "dir-glob".

See more at vercel/pkg#603 and
kevva/dir-glob#8
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.

None yet

5 participants