search

typesense / typesense-js

JavaScript / TypeScript client for Typesense
https://typesense.org/docs/api
Apache License 2.0
414 stars 75 forks source link

Lack of typings for logger #125

Closed michaelbromley closed 2 years ago

michaelbromley commented 2 years ago

Description

In the ConfigurationOptions interface, there is a logger property currently typed as any // todo. It would be good to get this properly typed so it is possible to implement custom logging without needing to reverse-engineer the library.

Metadata

Typsense Version: v1.3.0 (typesense-js)

OS: any

I'd like to have a go at adding typings for this.

michaelbromley commented 2 years ago

Ok, on further inspection I see that the logging is fully handled by the loglevel package, which is a direct dependency and also includes TS definitions. So I think it makes the most sense to directly expose the typings from that package in the config object, so as to not have to duplicate the typings inside the typesense-js code itself.

// Configuration.ts

import * as logger from 'loglevel'
import { Logger, LogLevelDesc } from 'loglevel'

// ...

export interface ConfigurationOptions {
  // ...
  logLevel?: LogLevelDesc
  logger?: Logger
}

Does that seem like a reasonable solution?

jasonbosco commented 2 years ago

@michaelbromley That sounds good to me. Thank you for helping with this.

michaelbromley commented 2 years ago

Great, I'll put in a PR.

One question - should I include the updated built files in /lib & /dist with the PR? I'm guessing so but just want to make sure.

jasonbosco commented 2 years ago

Could you skip the files in /lib and /dist in the PR, so the diff is easy to parse? I rebuild those before publishing to npm anyway.

jasonbosco commented 2 years ago

I've pushed this out in 1.4.0-2.