Skip to content
This repository was archived by the owner on Aug 30, 2021. It is now read-only.

Refactor mongodb connection#289

Merged
lirantal merged 5 commits into
meanjs:masterfrom
lirantal:refactor-mongodb-connection
Dec 27, 2014
Merged

Refactor mongodb connection#289
lirantal merged 5 commits into
meanjs:masterfrom
lirantal:refactor-mongodb-connection

Conversation

@lirantal
Copy link
Copy Markdown
Member

@lirantal lirantal commented Dec 4, 2014

This refactoring to mongodb connection handling will solve:

  1. username or password with special chars like @ won't be handled correctly to connect to mongodb via the connection string uri, so this refactor solves that
  2. exit on mongodb error instead of hang the nodejs app idle

@lirantal
Copy link
Copy Markdown
Member Author

lirantal commented Dec 4, 2014

Example:
refactor-mongodb-connection-1

@lirantal
Copy link
Copy Markdown
Member Author

@roieki @NeverOddOrEven what do you think?

@lirantal
Copy link
Copy Markdown
Member Author

@Shrizzy @amoshaviv care to take a review here? it looks like a much needed change IMHO and I want to merge

Comment thread config/env/production.js
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why so many process URIs? In my opinion, let the user change the one included if necessary, but no need to include one for every database service.

@ilanbiala
Copy link
Copy Markdown
Member

@lirantal other than the couple comments that I put, LGTM. The nice printout is definitely more useful, but all the process.env stuff and the options IMO is unnecessary.

@lirantal
Copy link
Copy Markdown
Member Author

@ilanbiala thanks for reviewing, couple of comments:

  1. As you can see, I didn't introduce the several environment variable URI variations, they were there before, and if someone cared to approve it then probably for a reason. I do agree with you that it may be 'too magicful' trying to resolve the URI like that but that can be solved in another, more concise PR for that.
  2. The user/pass can't be specified in the URI if they include special characters like !@#$%^&* - that's one of the reasons why I refactored the db connection like that (it's a known problem/bug with mongoose URI parameter)

lirantal added a commit that referenced this pull request Dec 27, 2014
@lirantal lirantal merged commit 8e5dbbe into meanjs:master Dec 27, 2014
@NeverOddOrEven
Copy link
Copy Markdown
Contributor

I just went over this for what it's worth. I like this change a lot!

@lirantal
Copy link
Copy Markdown
Member Author

@NeverOddOrEven glad you liked it :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants