Repository navigation
Conversation
| constructor(name: any, config: any); | ||
| name: any; | ||
| config: any; | ||
| } |
There was a problem hiding this comment.
| constructor(name: any, config: any); | |
| name: any; | |
| config: any; | |
| } | |
| constructor(name: string, config: any); | |
| public readonly name: string; | |
| public readonly config: any; | |
| } |
and please type config
| import type amqp = require('amqplib'); | ||
| import pDefer = require('p-defer'); | ||
|
|
||
| declare class Producer { |
There was a problem hiding this comment.
Please private modifiers where appropriate.. checkRpcQueue / maybeAnswer / etc .. should be private.
| import { Connection } from './connection'; | ||
| import { ConnectionHooks, ConsumerHooks, ProducerHooks } from './hooks'; | ||
|
|
||
| declare function arnavmq(connection: any): arnavmq.Arnavmq; |
There was a problem hiding this comment.
why connection is any here?
| constructor(name: any, config: any); | ||
| name: any; | ||
| config: any; | ||
| } |
| * | ||
| * [queue: string] -> [correlationId: string] -> {responsePromise, timeoutId} | ||
| */ | ||
| amqpRPCQueues: Record< |
There was a problem hiding this comment.
| amqpRPCQueues: Record< | |
| private amqpRPCQueues: Record< |
|
|
||
| declare class Producer { | ||
| constructor(connection: Connection); | ||
| hooks: ProducerHooks; |
There was a problem hiding this comment.
| hooks: ProducerHooks; | |
| private hooks: ProducerHooks; |
There was a problem hiding this comment.
These are actually public so you can add hooks to the producer, and it's exposed on the arnavmq object.
| Record<string, { responsePromise: pDefer.DeferredPromise<unknown>; timeoutId: NodeJS.Timeout }> | ||
| >; | ||
| private _connection: Connection; | ||
| set connection(value: Connection); |
There was a problem hiding this comment.
don't think we should allow setting connection ..
| set connection(value: Connection); | |
| private set connection(value: Connection); |
There was a problem hiding this comment.
This is actually used internally to replace the connection so it's not really private.
This should be cleaned as part of #68 since the replaced collection is always the same instance anyways and it doesn't make sense to even have this option, but on it's singleton form now that's how it is.
node-arnavmq/src/modules/arnavmq.js
Line 49 in 35a8461
There was a problem hiding this comment.
I don't expect someone to do arnavmq.producer.connection = ..., and if so I would consider it as bad practice.
I think it should be ok to mark it as private, and if once we move to TypeScript, have the connection be accepted at the constructor and don't expose it.
yosiat
left a comment
yosiat
left a comment
There was a problem hiding this comment.
This PR is important to defining our external API, so it's important that we mark public/private correctly.
I think connection should be made private (#69 (comment)) prior to merging, but I won't block the merge here.
| "types": "types/index.d.ts", | ||
| "scripts": { | ||
| "lint": "eslint . && prettier -c .", | ||
| "lint": "eslint . && prettier -c . && tsc --project types/tsconfig.types.json", |
There was a problem hiding this comment.
how lint relates to types?
There was a problem hiding this comment.
It doesn't really build the project, just makes sure the types are well formed. I think it's too much to add a "build" step to it.
It is used internally, not privately, but it makes sense to make it private on the types.
4 participants
Tested the type with several projects I have locally and everything compiles.
I had to fight the compiler very hard until I got to a definition that worked everywhere.