yfinkelstein / node-zookeeper

node.js client for Apache Zookeeper
MIT License
479 stars 111 forks source link

Difficulty in reading documentation as a beginner #342

Open simonebenati opened 10 months ago

simonebenati commented 10 months ago

Is your feature request related to a problem? Please describe. I have difficulties reading the documentation and especially what each parameter does.

Describe the solution you'd like Could you please provide a brief explanation to each option and what it does or maybe a reference to official zookeeper docs?

Describe alternatives you've considered

Additional context

DavidVujic commented 10 months ago

Thank you for this feature request @simonebenati!

I agree, the docs are difficult to read and should be improved.

Here's some info that we have today about the parameters: https://github.com/yfinkelstein/node-zookeeper?tab=readme-ov-file#input-parameters

simonebenati commented 10 months ago

@DavidVujic Thank you for the swift reply, for example why is the .init method written after the .on method ? What does the init do? I believe it initializes the client.. then you want to connect to zookeeper. What do all the _cb variables do?

thank you, Simone

simonebenati commented 10 months ago

Also one thing, how do you handle disconnects and errors using the .on method?

DavidVujic commented 10 months ago

Good feedback, thank you! This is very useful for me when updating the docs.

Meanwhile, there is the example code that I believe would answer some of your questions:

https://github.com/yfinkelstein/node-zookeeper/blob/master/examples/wrapper.js#L25

TimShilov commented 3 months ago

I also just tried using this library and, unfortunately, I'm really struggling to understand how to use it. I believe documentation and TypeScript types can be improved a lot.

Right off the bat, the first example in README is this:

Create an instance of the ZooKeeper client:

function createClient(timeoutMs = 5000) {
    const config = {
        connect: host,
        timeout: timeoutMs,
        debug_level: ZooKeeper.constants.ZOO_LOG_LEVEL_WARN,
        host_order_deterministic: false,
    };

    return new ZooKeeper(config);
}

const client = createClient();

The client is ready when connected to a ZooKeeper server:

client.on('connect', () => {
    // start using the client
});

client.init(config);

What is the config in client.init(config);? 🤷‍♂️

The documentation says

The source code is documented with JSDoc code comments and TypeScript type declarations.

There is an "Input parameters" section with some descriptions, but there are lots of methods and only one "Input parameters" section with no mention of "are they input parameters of what?". 🤷‍♂️

So I thought I could just look up the JSDoc/types. However, while they are present, they are not helpful at all. Here is the example of what they look like for .init():

/**
 * @param {object|string} config
 * @returns {ZooKeeper}
 */
init(config: object | string): ZooKeeper;

There are lots of problems with such a signature:

  1. No description of what the method actually does or what the inputs are.
  2. The input type object provides no information about what that object is supposed to be.
  3. Due to the previous point, there's no auto-complete on any of the inputs.
  4. The README mentions lots of methods as "async/await enabled client methods" but the type declarations don't mention that any of the methods return a Promise. Due to that, the tooling (TypeScript LSP and ESLint) are not happy about using them with await and all the code using this library is full of warnings like 'await' has no effect on the type of this expression..
  5. The type object in TypeScript is not strict enough as almost everything in JavaScript/TypeScript is an Object. Due to that, init([]);, init(() => {});, init(new Error());, init(new Set());, init(new Map()); and many more examples like that are all valid inputs for that function, as far as TypeScript is concerned.

And I'm not even talking about cases like constructor(config: any); which are even less helpful. Zookeeper is a great tool, and this library seems great as well, but it is such a shame that the documentation is so unfriendly and uninviting.

I'm happy to try and submit a PR(s) to address some of the issues mentioned above if I get a chance and will be able to understand the library.

DavidVujic commented 3 months ago

Hello @TimShilov, this project already uses TypeScript enabled docs, and I am surprised that it doesn't seem to work for you.

Here's a recording I just made (my code editor both pops up docs/types and at the very bottom). The types are already there and should work, unless there is some bugs or other configuration that I have overlooked.

zookeeper-code-docs

The config is a dynamic object, yes. And the optional input is there to avoid breaking features. I agree that the config could be typed, yes. On the other hand, as you pasted in already: the docs for the config is right there.

DavidVujic commented 3 months ago

You should also be able to step into the implementation, to see the details of the function interface:

step-in-zk

DavidVujic commented 3 months ago

@TimShilov In the recordings I have made, I don't use any specific TS config. I just created a new project with npm init and then npm install zookeeper.

It seems that your setup is different, can you share that one and/or help me figure out what doesn't work for you?

DavidVujic commented 3 months ago

Adding another response about the dynamic options object passed in to the init function, for any new readers of this thread. The data format is already documented (even if it could be done a lot better, of course) here.

about the init function: https://github.com/yfinkelstein/node-zookeeper?tab=readme-ov-file#methods-asyncawait-enabled-client-methods

So, what's the options? You will find information about the data here, in the Input parameters section: https://github.com/yfinkelstein/node-zookeeper?tab=readme-ov-file#input-parameters

I think this, together with my examples from the previous comments, should cover all the things raised by @TimShilov except the fact that the options data type is a JS object and not strictly typed.