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

Slight improvement: validate configuration #113

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

tahsinature
Copy link

@tahsinature tahsinature commented Nov 16, 2021

Summary

Improve local development slightly.

Checklist

  • add nodemon as dev dependency as it was producing nodemon not found error.
  • add validator for the necessary configuration.
  • make application (src/index.js) file a bit organized

Copy link
Member

@iamstevedavis iamstevedavis left a comment

Choose a reason for hiding this comment

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

Hey Mohammad,

Thanks for the contribution, lots of good stuff here! I left a few thoughtful comments that I noticed up front, feel free to rebuke.

@@ -1,5 +1,5 @@
import jwt from "jsonwebtoken";
import { jwtConfig } from "../../../config";
Copy link
Member

Choose a reason for hiding this comment

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

I think we want to stick with the approach of importing what we need for config values!

},
done
);
request(app).post("/api/appointments").send(appointment).expect(
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure the linting is handling this correctly but generally we have tried to do one new line per chained call.

Copy link
Author

Choose a reason for hiding this comment

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

It was for the linter. The printWidth property. It wasn't there.
I just added it.
So yeah now it's breaking down the chain calls to multiple lines if it's more than 80 chars.

}

start() {
validateConfig();
Copy link
Member

Choose a reason for hiding this comment

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

This will display the config error and won't start but do we want it to be because of the exception or because of the error? For example, if you add a try/catch block around validate config you would see

Error: "twilioConfig.number" is required
    at module.exports.validateConfig (/media/wwan/git/sanctuary-patient-api/src/config.js:47:20)
    at Application.start (/media/wwan/git/sanctuary-patient-api/src/index.js:20:7)
    at Object.<anonymous> (/media/wwan/git/sanctuary-patient-api/src/index.js:63:13)
    at Module._compile (node:internal/modules/cjs/loader:1095:14)
    at Module._compile (/media/wwan/git/sanctuary-patient-api/node_modules/pirates/lib/index.js:99:24)
    at Module._extensions..js (node:internal/modules/cjs/loader:1147:10)
    at Object.newLoader [as .js] (/media/wwan/git/sanctuary-patient-api/node_modules/pirates/lib/index.js:104:7)
    at Module.load (node:internal/modules/cjs/loader:975:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
[nodemon] app crashed - waiting for file changes before starting...

where right now we see:

/media/wwan/git/sanctuary-patient-api/src/config.js:50
  if (error) throw new Error(error.message);
             ^

Error: "twilioConfig.number" is required
    at module.exports.validateConfig (/media/wwan/git/sanctuary-patient-api/src/config.js:47:20)
    at Application.start (/media/wwan/git/sanctuary-patient-api/src/index.js:18:7)
    at Object.<anonymous> (/media/wwan/git/sanctuary-patient-api/src/index.js:57:13)
    at Module._compile (node:internal/modules/cjs/loader:1095:14)
    at Module._compile (/media/wwan/git/sanctuary-patient-api/node_modules/pirates/lib/index.js:99:24)
    at Module._extensions..js (node:internal/modules/cjs/loader:1147:10)
    at Object.newLoader [as .js] (/media/wwan/git/sanctuary-patient-api/node_modules/pirates/lib/index.js:104:7)
    at Module.load (node:internal/modules/cjs/loader:975:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)

Node.js v17.0.1

Copy link
Author

Choose a reason for hiding this comment

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

If we add try/catch block, the problem is, it will continue the program.
But if we do not run the app at all, I think it will be more helpful for developers to figure out why.
What do you think?

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