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

Add type declarations. #69

Merged
merged 6 commits into from
Mar 11, 2024
Merged

Add type declarations. #69

merged 6 commits into from
Mar 11, 2024

Conversation

ramhr
Copy link
Contributor

@ramhr ramhr commented Mar 6, 2024

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.

tsconfig.json Show resolved Hide resolved
types/index.d.ts Outdated Show resolved Hide resolved
types/modules/arnavmq.d.ts Outdated Show resolved Hide resolved
types/modules/channels.d.ts Outdated Show resolved Hide resolved
types/modules/channels.d.ts Outdated Show resolved Hide resolved
Comment on lines 23 to 26
constructor(name: any, config: any);
name: any;
config: any;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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

Copy link
Contributor

Choose a reason for hiding this comment

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

@ramhr what about this ?

types/modules/consumer.d.ts Outdated Show resolved Hide resolved
types/modules/consumer.d.ts Outdated Show resolved Hide resolved
types/modules/hooks/base_hooks.d.ts Outdated Show resolved Hide resolved
import type amqp = require('amqplib');
import pDefer = require('p-defer');

declare class Producer {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please private modifiers where appropriate.. checkRpcQueue / maybeAnswer / etc .. should be private.

@yosiat yosiat assigned ramhr and unassigned yosiat Mar 6, 2024
@ramhr ramhr assigned yosiat and unassigned ramhr Mar 7, 2024
import { Connection } from './connection';
import { ConnectionHooks, ConsumerHooks, ProducerHooks } from './hooks';

declare function arnavmq(connection: any): arnavmq.Arnavmq;
Copy link
Contributor

Choose a reason for hiding this comment

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

why connection is any here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, missed it

Comment on lines 23 to 26
constructor(name: any, config: any);
name: any;
config: any;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@ramhr what about this ?

*
* [queue: string] -> [correlationId: string] -> {responsePromise, timeoutId}
*/
amqpRPCQueues: Record<
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
amqpRPCQueues: Record<
private amqpRPCQueues: Record<

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also added readonly


declare class Producer {
constructor(connection: Connection);
hooks: ProducerHooks;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
hooks: ProducerHooks;
private hooks: ProducerHooks;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

don't think we should allow setting connection ..

Suggested change
set connection(value: Connection);
private set connection(value: Connection);

Copy link
Contributor Author

@ramhr ramhr Mar 10, 2024

Choose a reason for hiding this comment

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

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.

instance.connection = connection;

Copy link
Contributor

Choose a reason for hiding this comment

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

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 yosiat assigned ramhr and unassigned yosiat Mar 10, 2024
@ramhr ramhr assigned yosiat and unassigned yosiat Mar 10, 2024
Copy link
Contributor

@yosiat yosiat left a comment

Choose a reason for hiding this comment

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

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.

@yosiat yosiat removed their assignment Mar 10, 2024
"scripts": {
"lint": "eslint . && prettier -c .",
"lint": "eslint . && prettier -c . && tsc --project types/tsconfig.types.json",
Copy link
Member

@shamil shamil Mar 10, 2024

Choose a reason for hiding this comment

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

how lint relates to types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@shamil shamil removed their assignment Mar 10, 2024
It is used internally, not privately, but it makes sense to make it private on the types.
@ramhr ramhr merged commit 013e8c1 into master Mar 11, 2024
4 checks passed
@ramhr ramhr deleted the types branch March 11, 2024 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants