(fix) [NAN-644] refactor logger and some environment detection logic out of #1913
Conversation
| export function isBasicAuthEnabled() { | ||
| return !isCloud() && process.env['NANGO_DASHBOARD_USERNAME'] && process.env['NANGO_DASHBOARD_PASSWORD']; | ||
| } | ||
|
|
There was a problem hiding this comment.
Debatable if these two below should be here or not.
…vironment-detection-logic-out-of
| @@ -10,6 +10,8 @@ import type { RawDataRecordResult, DataRecord, DataRecordWithMetadata, RecordWra | |||
| import db from '../db/database.js'; | |||
| import util from 'util'; | |||
|
|
|||
| const { logger } = new Logger('Encryption.Manager'); | |||
|
|
|||
| interface DataRecordJson { | |||
| encryptedValue: string; | |||
| [key: string]: any; | |||
There was a problem hiding this comment.
I am tempted to also move encryption.manager into this new package. After all it was in utils before. The question is how to we deal with types?
There was a problem hiding this comment.
Reiterating that I think this package would be better called utils. And personally I would create all those internal packages (I feel we are going to have a bunch) into a subdirectory of /packages (for example: /packages/libs or packages/internals to differentiate the end products (cloud services, published packages, etc...) to the stuff only used internally
There was a problem hiding this comment.
Nice starts, very glad we are moving on this.
I have a few comments, starting from almost scratch is a good time to start with alignment too 👍🏻
I think it's also missing few things:
- main package.json workspaces property
- Nodemon listen to internals/dist on all related packages, so it restart when modified
- Our many Dockerfiles are not updated and I'm actually surprised it passed the CI
packages/internals/lib/logger.ts
Outdated
| return winston.format.printf((info) => { | ||
| return `${info['timestamp']} [${info.level.toUpperCase()}]${service ? ` [${service}] ` : ''}${info.message}`; | ||
| }); | ||
| }; | ||
|
|
||
| class NangoLogger { |
There was a problem hiding this comment.
I think I want to question our use of Class, most of the time they seems to hold no state, here it's just a function.
export function getLogger(server?: string) {
winston.createLogger({
levels: winston.config.syslog.levels,
format: winston.format.combine(winston.format.timestamp(), nangoLogFormat),
format: winston.format.combine(winston.format.timestamp(), nangoLogFormat(service)),
transports: [new winston.transports.Console({ level: process.env['LOG_LEVEL'] || 'info' })]
});
}
export const logger = getLogger();
packages/internals/lib/logger.ts
Outdated
| transports: [new winston.transports.Console({ level: process.env['LOG_LEVEL'] || 'info' })] | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| export default new NangoLogger().logger; | ||
| export default NangoLogger; |
There was a problem hiding this comment.
I would also like to discuss our use of export default.
I find it a bit inconvenient to find import across the codebase, import name is not enforced making it harder to have a consistent way of naming things. Plus if we need to export more then we have a mix of default + named.
For example here, vscode can't autocomplete Logger and only find NangoLogger which would differ from what you have wrote.
…ection-logic-out-of' of github.com:NangoHQ/nango into khaliq/nan-644-refactor-logger-and-some-environment-detection-logic-out-of
| @@ -28,7 +28,7 @@ jobs: | |||
| run: | | |||
| # Build, install CLI and verify it can run | |||
| npm install | |||
| npm run build -w @nangohq/node -w @nangohq/shared -w nango | |||
| npm run build -w @nangohq/node -w @nangohq/utils -w @nangohq/shared -w nango | |||
There was a problem hiding this comment.
I think we could just npm run ts-build
There was a problem hiding this comment.
I think you forgot to change that, or put back a build command in utils/package.json, but I think using ts-build is safer
There was a problem hiding this comment.
Building everything seemed like overkill rather than just building what we need exactly.
I’ll add back in the specific build command which was there originally in utils. I think it’s handy to have specific build commands in each package regardless
khaliqgant
deleted the
khaliq/nan-644-refactor-logger-and-some-environment-detection-logic-out-of
branch
Describe your changes
Add
internalsutilspackage and move logger and some environment detection logic thereIssue ticket number and link
NAN-644
Checklist before requesting a review (skip if just adding/editing APIs & templates)