Skip to content
This repository has been archived by the owner on Jul 31, 2019. It is now read-only.

Fix #1908: Issue 1908 glob update #1938

Merged
merged 7 commits into from Apr 13, 2017

Conversation

badrmodoukh
Copy link
Contributor

I have updated the glob dependency
I tested using "npm install" and all seems to work fine from my end
I also ran "vagrant up" and loaded thimble and all worked fine

@Pomax
Copy link
Contributor

Pomax commented Apr 4, 2017

difference between v7 and v6: https://github.com/isaacs/node-glob/blob/master/changelog.md#70

@Pomax Pomax self-requested a review April 4, 2017 01:02
Copy link
Contributor

@Pomax Pomax left a comment

Choose a reason for hiding this comment

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

The difference between glob 6 and 7 kicks in when options.cwd is used, where it will throw an error if the "cwd" argument is not a directory. As it so happens, we use options.cwd in https://github.com/mozilla/thimble.mozilla.org/blob/fa3dde088dc016a562520e62a9df1f5558ba8f70/server/lib/utils.js so we want to make sure we have an error handler there now.

@humphd
Copy link
Contributor

humphd commented Apr 4, 2017

Nice catch @Pomax. @badrmodoukh can you do a fix for this in this PR?

@badrmodoukh
Copy link
Contributor Author

@Pomax @humphd I added the error handler in the utils.js file
Sorry for the many commits I made

@humphd
Copy link
Contributor

humphd commented Apr 5, 2017

@badrmodoukh if you need to commit something, and don't want to add more commits, use git commit --amend --no-edit and it will just combine the previous commit with this new stuff. This is only the right thing to do when you're working on your own stuff, and haven't shared the commits with other people, since it will replace the old commit, thus destroying history (i.e., you don't want to alter public history, but private history is fine to rewrite).

nodir: true
}).map(file => path.join(root, file));
}
catch (err) { }
Copy link
Contributor

Choose a reason for hiding this comment

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

Never, ever, ever. Not ever. Never. Not even once.

This pattern will cause you endless amounts of grief, when you have some bug where it silently fails and you don't know why. Always do something with the error.

@badrmodoukh
Copy link
Contributor Author

@humphd sorry about that
I just modified it right now

@Pomax
Copy link
Contributor

Pomax commented Apr 13, 2017

this is good to go, thanks @badrmodoukh

@Pomax Pomax merged commit 1eba16d into mozilla:master Apr 13, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants