-
Notifications
You must be signed in to change notification settings - Fork 4
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
#102 into [email protected] 🐘 add logger support #131
base: [email protected]
Are you sure you want to change the base?
Conversation
# Conflicts: # src/core/graphql/createGraphQLRoutes/createGraphQLRoutes.test.ts # src/core/graphql/createGraphQLRoutes/createGraphQLRoutes.ts # src/core/middlewares/notFoundMiddleware/notFoundMiddleware.test.ts # src/core/rest/createRestRoutes/createRestRoutes.test.ts # src/core/rest/createRestRoutes/createRestRoutes.ts # src/server/createGraphQLMockServer/createGraphQLMockServer.ts # src/server/createMockServer/createMockServer.ts # src/server/createRestMockServer/createRestMockServer.ts
…, update logger logic in createGraphQLRoutes
src/utils/helpers/logger/helpers/coloredString/getRestMethodColoredString.ts
Outdated
Show resolved
Hide resolved
src/utils/helpers/logger/callRequestLogger/callRequestLogger.ts
Outdated
Show resolved
Hide resolved
src/utils/helpers/date/formatUnixTimestamp/formatUnixTimestamp.ts
Outdated
Show resolved
Hide resolved
src/core/middlewares/requestInfoMiddleware/requestInfoMiddleware.ts
Outdated
Show resolved
Hide resolved
if (logger.tokens?.meta?.url) { | ||
console.info(`${metaPrefix}[${c.yellow('url')}] ${url}`); | ||
} | ||
if (logger.tokens?.meta?.method) { | ||
console.info(`${metaPrefix}[${c.red('method')}] ${getRestMethodColoredString(method)}`); | ||
} | ||
if (logger.tokens?.meta?.graphQLOperationType) { | ||
console.info(`${metaPrefix}[${c.blue('graphQLOperationType')}] ${graphQLOperationType}`); | ||
} | ||
if (logger.tokens?.meta?.graphQLOperationName) { | ||
console.info(`${metaPrefix}[${c.green('graphQLOperationName')}] ${graphQLOperationName}`); | ||
} | ||
if (logger.tokens?.meta?.statusCode) { | ||
console.info( | ||
`${metaPrefix}[${c.yellowBright('statusCode')}] ${getStatusCodeColoredString(statusCode)}` | ||
); | ||
} | ||
if (logger.tokens?.meta?.id) { | ||
console.info(`${metaPrefix}[${c.greenBright('id')}] ${id}`); | ||
} | ||
if (logger.tokens?.meta?.unixTimestamp) { | ||
console.info(`${metaPrefix}[${c.blue('timestamp')}] ${formatUnixTimestamp(unixTimestamp)}`); | ||
} | ||
|
||
if (logger.tokens?.entities?.headers) { | ||
console.info(`${entitiesPrefix}[${c.cyan('headers')}]\n${JSON.stringify(headers, null, 2)}`); | ||
} | ||
if (logger.tokens?.entities?.cookies) { | ||
console.info(`${entitiesPrefix}[${c.magenta('cookies')}]\n${JSON.stringify(cookies, null, 2)}`); | ||
} | ||
if (logger.tokens?.entities?.query) { | ||
console.info(`${entitiesPrefix}[${c.cyanBright('query')}]\n${JSON.stringify(query, null, 2)}`); | ||
} | ||
if (logger.tokens?.entities?.params) { | ||
console.info(`${entitiesPrefix}[${c.red('params')}]\n${JSON.stringify(params, null, 2)}`); | ||
} | ||
if (logger.tokens?.entities?.variables) { | ||
console.info( | ||
`${entitiesPrefix}[${c.yellow('variables')}]\n${JSON.stringify(variables, null, 2)}` | ||
); | ||
} | ||
if (logger.tokens?.entities?.body) { | ||
console.info(`${entitiesPrefix}[${c.green('body')}]\n${JSON.stringify(body, null, 2)}`); | ||
} | ||
|
||
if (logger.tokens?.data) { | ||
console.info(`${prefix}[${c.yellow('data')}]\n${JSON.stringify(data, null, 2)}`); | ||
} | ||
|
||
console.info('\n\n'); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
У console есть методы group и groupEnd. Можно их использовать, чтобы объединять сущности одного уровня (например entities)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
переписал логику так, что теперь эта функция возвращает объект с токенами, который потом мы один раз console.dir-им
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А отличается ли console.dir от console.info(JSON.stringify()) если мы говорим о NodeJs?
const isValidGraphQLRequest = | ||
graphQLInput && | ||
graphQLInput.query && | ||
graphQLQuery?.operationType && | ||
graphQLQuery.operationName; | ||
request.graphQL = isValidGraphQLRequest | ||
? { | ||
operationType: graphQLQuery.operationType as GraphQLOperationType, | ||
operationName: graphQLQuery.operationName, | ||
variables: graphQLInput.variables | ||
} | ||
: null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
operationType и operationName в типе ResponseLogFunctionParams могут быть undefined, хотя по сути не могут
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
почему не могут? request.graphQL?.operationType будет undefined т.к. graphQL === null
const requestLogger = mockServerConfig.loggers?.request; | ||
if (requestLogger) { | ||
await callRequestLogger({ request, logger: requestLogger }); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Получается, что requestLogger вызывается независимо от того, был найден нужный конфиг или нет. Мб тогда вызывать его в одном месте, а не в двух?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
от этого дублирования не уйти, если вынести вызов callRequestLogger из createRestRoutes, createGraphQLRoutes и notFoundMiddleware, то придётся добавлять одинаковый интерцептор в createMockServer, createRestMockServer и createGraphQLMockServer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Да, но тогда у нас хотя бы будут все вызовы подобных вещей в одном месте - те же interceptor-ы вызываются как раз в create...
функциях
# Conflicts: # .eslintrc.js # src/core/graphql/createGraphQLRoutes/createGraphQLRoutes.ts # src/core/middlewares/notFoundMiddleware/notFoundMiddleware.ts # src/core/rest/createRestRoutes/createRestRoutes.ts # src/server/createGraphQLMockServer/createGraphQLMockServer.ts # src/server/createMockServer/createMockServer.ts # src/server/createRestMockServer/createRestMockServer.ts # src/utils/helpers/interceptors/callRequestInterceptor/callRequestInterceptor.test.ts # src/utils/types/graphql.ts # src/utils/types/rest.ts # src/utils/types/utils.ts
// TODO: add loggers validation | ||
// import { validateMockServerConfig } from './validateMockServerConfig/validateMockServerConfig'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А что с этим?
import type { ParamsDictionary } from 'express-serve-static-core'; | ||
import type { IncomingHttpHeaders } from 'http'; | ||
import type { ParsedQs } from 'qs'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
У express-serve-static-core
есть тип Query - я думаю более наглядно будет использовать его, нежели тип из qs, который является подзависимостью express
rest?: RestConfig; | ||
graphql?: GraphqlConfig; | ||
database?: DatabaseConfig; | ||
loggers?: Loggers; | ||
} | ||
|
||
export interface RestMockServerConfig extends BaseMockServerConfig { | ||
configs?: RestRequestConfig[]; | ||
database?: DatabaseConfig; | ||
loggers?: Loggers<'rest'>; | ||
} | ||
|
||
export interface GraphQLMockServerConfig extends BaseMockServerConfig { | ||
configs?: GraphQLRequestConfig[]; | ||
database?: DatabaseConfig; | ||
loggers?: Loggers<'graphql'>; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А для database логгера не будет?
export interface LoggerTokenFlags< | ||
Type extends LoggerType = LoggerType, | ||
API extends LoggerAPI = LoggerAPI | ||
> { | ||
meta?: { | ||
unixTimestamp?: boolean; | ||
id?: boolean; | ||
method?: boolean; | ||
graphQLOperationType?: API extends 'graphql' ? boolean : never; | ||
graphQLOperationName?: API extends 'graphql' ? boolean : never; | ||
url?: boolean; | ||
statusCode?: Type extends 'response' ? boolean : never; | ||
}; | ||
entities?: { | ||
headers?: boolean; | ||
cookies?: any; | ||
query?: boolean; | ||
params?: boolean; | ||
variables?: API extends 'graphql' ? boolean : never; | ||
body?: boolean; | ||
}; | ||
data?: Type extends 'response' ? boolean : never; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
entities.cookies так и должно быть any?
const requestLogger = mockServerConfig.loggers?.request; | ||
if (requestLogger) { | ||
await callRequestLogger({ request, logger: requestLogger }); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Да, но тогда у нас хотя бы будут все вызовы подобных вещей в одном месте - те же interceptor-ы вызываются как раз в create...
функциях
@@ -16,10 +16,12 @@ module.exports = { | |||
'@typescript-eslint/no-unsafe-enum-comparison': 'off', | |||
'@typescript-eslint/no-var-requires': 'off', | |||
'@typescript-eslint/naming-convention': 'off', | |||
'@typescript-eslint/no-namespace': 'off', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'@typescript-eslint/no-namespace': ['error', { allowDeclarations: true }]
- я думаю лучше сделать так, чтобы минимально снять ограничения, а не полностью отключать правило
const getTokens: GetTokens<'response'> = ({ logger, tokenValues }) => { | ||
const { | ||
meta: { | ||
unixTimestamp, | ||
id, | ||
statusCode, | ||
url, | ||
method, | ||
graphQLOperationType, | ||
graphQLOperationName | ||
}, | ||
entities: { headers, cookies, query, params, variables, body }, | ||
data | ||
} = tokenValues; | ||
|
||
if (!logger.tokens || !Object.keys(logger.tokens).length) { | ||
return { | ||
type: 'response', | ||
meta: { | ||
timestamp: getTimestamp(unixTimestamp), | ||
id, | ||
statusCode | ||
} | ||
}; | ||
} | ||
|
||
return { | ||
type: 'response', | ||
...(logger.tokens.meta && { | ||
meta: { | ||
...(logger.tokens.meta.unixTimestamp && { timestamp: getTimestamp(unixTimestamp) }), | ||
...(logger.tokens.meta.id && { id }), | ||
...(logger.tokens.meta.method && { method }), | ||
...(logger.tokens.meta.graphQLOperationType && { graphQLOperationType }), | ||
...(logger.tokens.meta.graphQLOperationName && { graphQLOperationName }), | ||
...(logger.tokens.meta.url && { url }), | ||
...(logger.tokens.meta.statusCode && { statusCode }) | ||
} | ||
}), | ||
...(logger.tokens.entities && { | ||
entities: { | ||
...(logger.tokens.entities.headers && { headers }), | ||
...(logger.tokens.entities.cookies && { cookies }), | ||
...(logger.tokens.entities.query && { query }), | ||
...(logger.tokens.entities.params && { params }), | ||
...(logger.tokens.entities.variables && { variables }), | ||
...(logger.tokens.entities.body && { body }) | ||
} | ||
}), | ||
...(logger.tokens.data && { | ||
data | ||
}) | ||
}; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Мне кажется, что можно упростить апи этой функции - нет смысла передавать все loggerParams, если функции по сути нужны только tokenFlags и tokenValues. И так будет легче понять ее смысл
tokenValues: { | ||
type: 'response', | ||
meta: { | ||
unixTimestamp: Date.now(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
request.timestamp?
if (logger.logFunction) { | ||
await logger.logFunction(loggerParams); | ||
return; | ||
} | ||
const tokens = await getTokens(loggerParams); | ||
console.dir(tokens, { depth: null }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
А нормально ли, что дефолтный console и logFunction оба сработают?
export interface LoggerTokenValues< | ||
Type extends LoggerType = LoggerType, | ||
API extends LoggerAPI = LoggerAPI | ||
> { | ||
type: Type; | ||
meta: { | ||
unixTimestamp: number; | ||
id: number | undefined; | ||
method: RestMethod; | ||
graphQLOperationType: API extends 'graphql' ? GraphQLOperationType | undefined : never; | ||
graphQLOperationName: API extends 'graphql' ? GraphQLOperationName | undefined : never; | ||
url: string; | ||
statusCode?: Type extends 'response' ? number : never; | ||
}; | ||
entities: { | ||
headers: Headers; | ||
cookies: Cookies; | ||
query: Query; | ||
params: Params; | ||
variables: API extends 'graphql' ? PlainObject | undefined : never; | ||
body: any; | ||
}; | ||
data?: Type extends 'response' ? any : never; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
С подобным типом автодополнение IDE не очень хорошо работает (для REST, например, он предложит graphql свойства, а для request свойства response). Т.к. мы отталкиваемся в первую очередь от удобства использования, а не удобства разработки, то предлагаю сделать 4 типа LoggerTokenValues (request,response - для каждого из них rest и graphql). А из этих 4 типов создать типы LoggerTokenFlags будет совсем просто - это по сути DeepPartial, только типами всех свойств станет boolean
Resolves #102