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

chore(dx): Improved DX for contributors to help get started #30

Closed
wants to merge 3 commits into from
Closed

chore(dx): Improved DX for contributors to help get started #30

wants to merge 3 commits into from

Conversation

coltonehrman
Copy link
Contributor

@coltonehrman coltonehrman commented Dec 25, 2023

Why?

I made this PR to ease the initial setup of the repo locally, as the code was not configured correctly to get started and was missing a few things. This should help anyone else who's trying to setup locally and contribute :)

Chores

  • Made the DB config more configurable to allow for localhost
  • Added info to the README to guide the user on setting up DB locally

…app locally.

- Made the DB config more configurable to allow for localhost
- Added info to the README to guide the user on setting up DB locally
@labrocadabro
Copy link
Owner

Thank you so much for this!

I'd like to make a few changes:

  1. in the .env, I think it's best to have a DB_LOCAL variable set to true or false. This will make it easier to separate the two connection cases of localhost and Atlas/remote. Some people may want to use their own Atlas account during development, so it's not just about production.

I'm thinking of something along the lines of:

const isLocal = process.env.DB_LOCAL;
if (isLocal.toLowerCase() === "true") {
// build and return localhost URI
} else {
// build and return remote URI
}
  1. Instead of DB_CLUSTER, let's go with DB_LOCAL_PORT and DB_REMOTE_URI (with a commented example of what that would look like eg cluster0.58qh2.mongodb.net). This makes it the most flexible, so if someone happens to be using a remote connection string that isn't Atlas, or a localhost port other than 27017, it can still work.

  2. I'd prefer to leave fields blank in the .env.example unless they're defaults that are unlikely to be changed (like the localhost port). When you're scanning the file, that makes it a little more obvious what you need to fill in.

  3. Can you make the local DB instructions with two options, one with Docker and one without? Not everyone can run Docker - I did 100Devs on a 12 year old computer that couldn't even install Docker Desktop. 😄

  4. Please export connectDB and remove the export default from config/db.js

@coltonehrman
Copy link
Contributor Author

  1. in the .env, I think it's best to have a DB_LOCAL variable set to true or false. This will make it easier to separate the two connection cases of localhost and Atlas/remote. Some people may want to use their own Atlas account during development, so it's not just about production.

That makes sense. After thinking about it a bit. I'm wondering if we should just have a single env variable MONGODB_URI to set the whole string instead? I feel like this would be easier to control and maintain. Let me know what you think and I can update the PR.

@labrocadabro
Copy link
Owner

labrocadabro commented Dec 30, 2023 via email

- Simplifies DB connection across app
- Connection between local vs remote DB is abstracted
@coltonehrman
Copy link
Contributor Author

You know, you're probably right. Let's go with that, it makes things far simpler. 👍

On Fri, Dec 29, 2023 at 11:25 PM Colton Ehrman @.> wrote: 1. in the .env, I think it's best to have a DB_LOCAL variable set to true or false. This will make it easier to separate the two connection cases of localhost and Atlas/remote. Some people may want to use their own Atlas account during development, so it's not just about production. That makes sense. After thinking about it a bit. I'm wondering if we should just have a single env variable MONGODB_URI to set the whole string instead? I feel like this would be easier to control and maintain. Let me know what you think and I can update the PR. — Reply to this email directly, view it on GitHub <#30 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMGUD6JVKOCEZSU36PWR53YL6CSXAVCNFSM6AAAAABBCTTIJGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZSGQZTSOBYHE . You are receiving this because you commented.Message ID: @.>

I have updated :)

@coltonehrman
Copy link
Contributor Author

@labrocadabro Do you plan on merging this? It's not easy to make changes without these code changes merged. I have to "hack" the DB url otherwise.

@labrocadabro
Copy link
Owner

I apologize, this is a more complex issue, so I've been punting on it. It also just registered that you are connecting through localhost and therefore this change is urgent for you, sorry!

I'd like to move the db connection documentation from the readme to a separate file (we eventually need proper docs), which you can just link from the readme. This will keep it from getting too long and not everyone needs to work with MongoDB locally. I've also made changes to the readme recently, which you'll need to take into account.

If you can, instead of having a default URI for MONGODB_URI, just put two comments above it, one as an example of a localhost connection string, and one as an example of a remote (Atlas) connection.

We don't really need the function getDBUri anymore, we can just use process.env.MONGODB_URI directly.

Once these changes are implemented, I can merge this one.

@labrocadabro
Copy link
Owner

In the interest of having this complete so you (and anyone else using localhost) can get this working, I've made the edits myself. I didn't include the documentation, which is now probably more appropriate as part of #44, which @KhuloodHassan is working on. I've made them aware of the documentation you've written so it can be incorporated.

As that should take care of everything that this PR addresses, I'm closing this one.

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.

2 participants