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

BufferedMetricsLogger can't be used as a type #119

Closed
jfirebaugh opened this issue Oct 31, 2024 · 9 comments · Fixed by #120
Closed

BufferedMetricsLogger can't be used as a type #119

jfirebaugh opened this issue Oct 31, 2024 · 9 comments · Fixed by #120

Comments

@jfirebaugh
Copy link

jfirebaugh commented Oct 31, 2024

The following should work (TypeScript playground link):

import { BufferedMetricsLogger } from 'datadog-metrics'

function useLogger(logger: BufferedMetricsLogger) {
   logger.gauge('key', 0)
}

useLogger(new BufferedMetricsLogger())

However, it produces the following error:

'BufferedMetricsLogger' refers to a value, but is being used as a type here. Did you mean 'typeof BufferedMetricsLogger'?

(The suggestion the error message provides is not what I meant -- it generates different errors.)

@jfirebaugh
Copy link
Author

jfirebaugh commented Oct 31, 2024

The correct way to declare BufferedMetricsLogger in index.d.ts is given here:

export declare const BufferedMetricsLogger = loggers.BufferedMetricsLogger;
export declare type BufferedMetricsLogger = loggers.BufferedMetricsLogger;

@jfirebaugh
Copy link
Author

jfirebaugh commented Oct 31, 2024

@Mr0grog
Copy link
Collaborator

Mr0grog commented Oct 31, 2024

Hey @jfirebaugh! 👋

Thanks for filing this! Guess I should have included a test of the types when I added support for generating them here. 😓 All the typings here are generated from JSDoc, so I need to figure out the right incantation to get it to do this. Will see what I can figure that out ASAP.

@jfirebaugh
Copy link
Author

Hi Rob! Good to see you around. 😄

Mr0grog added a commit that referenced this issue Oct 31, 2024
Even though we started generating TypeScript types from the JSDoc a while back, it turns out `BufferedMetricsLogger` was getting exported as a value and not a type, which technically works in TS, but is definitely not what you want. This changes up our syntax a bit to get the compiler to do the right thing, and adds a basic test of the types to make sure we don't regress.

Fixes #119.
@Mr0grog
Copy link
Collaborator

Mr0grog commented Oct 31, 2024

@jfirebaugh I think I’ve got a working solution over in #120; are you able to test that that works for you, or do you need a pre-release build on NPM?

@jfirebaugh
Copy link
Author

I think I would need a pre-release build. I tried putting "datadog-metrics": "dbader/node-datadog-metrics#119-if-you-want-types-you-probably-want-actual-types" in my package.json but the resulting installed package does not have declaration files. Nor does it have a tsconfig.json, so running cd node_modules/datadog-metrics && npm run build-types doesn't work either.

Mr0grog added a commit that referenced this issue Nov 1, 2024
Even though we started generating TypeScript types from the JSDoc a while back, it turns out `BufferedMetricsLogger` was getting exported as a value and not a type, which technically works in TS, but is definitely not what you want. This changes up our syntax a bit to get the compiler to do the right thing, and adds a basic test of the types to make sure we don't regress.

Fixes #119.
Mr0grog added a commit that referenced this issue Nov 1, 2024
Even though we started generating TypeScript types from the JSDoc a while back, it turns out `BufferedMetricsLogger` was getting exported as a *value* and not a type. That technically works in TS, but requires some weird syntax and is definitely not what you want. This changes up our imports and exports a bit to get the TypeScript compiler to do the right thing, and adds a basic test of the types to make sure we don't regress.

See #119.
@Mr0grog Mr0grog reopened this Nov 1, 2024
@Mr0grog
Copy link
Collaborator

Mr0grog commented Nov 1, 2024

@jfirebaugh OK, you should be able to test with 0.11.4-a1. (It’s clearly been too long since I published a pre-release on NPM, because I totally botched this one, hence version 0.11.4 instead of 0.11.3. 😩)

@jfirebaugh
Copy link
Author

0.11.4-a.1 works, thanks!

@Mr0grog
Copy link
Collaborator

Mr0grog commented Nov 10, 2024

Sorry it took me a while to publish a production release here. This is now available in v0.11.4.

@Mr0grog Mr0grog closed this as completed Nov 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants