Skip to content

fix(types): correct CommonJS export in @fastify/otel typings #30

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

fix(types): correct CommonJS export in @fastify/otel typings #30

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 17 additions & 7 deletions index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,19 @@ type FastifyError = any

type HookHandlerDoneFunction = <TError extends Error = FastifyError>(err?: TError) => void

export type FastifyPlugin = (
type FastifyPlugin = (
instance: FastifyInstance,
opts: any,
done: HookHandlerDoneFunction,
) => unknown | Promise<unknown>

export interface FastifyOtelOptions {}
export interface FastifyOtelInstrumentationOpts extends InstrumentationConfig {
interface FastifyOtelOptions {}
interface FastifyOtelInstrumentationOpts extends InstrumentationConfig {
servername?: string
registerOnInitialization?: boolean
}

export interface FastifyInstance {
interface FastifyInstance {
version: string;
register: (plugin: any) => FastifyInstance;
after: (listener?: (err: Error) => void) => FastifyInstance;
Expand All @@ -49,11 +49,21 @@ export interface FastifyInstance {
}

declare class FastifyOtelInstrumentation<Config extends FastifyOtelInstrumentationOpts = FastifyOtelInstrumentationOpts> extends InstrumentationBase<Config> {
static FastifyInstrumentation: FastifyOtelInstrumentation
static FastifyInstrumentation: typeof FastifyOtelInstrumentation
constructor (config?: FastifyOtelInstrumentationOpts)
init (): InstrumentationNodeModuleDefinition[]
plugin (): FastifyPlugin
}

export default FastifyOtelInstrumentation
export { FastifyOtelInstrumentation }
Copy link
Contributor Author

@bcomnes bcomnes Mar 17, 2025

Choose a reason for hiding this comment

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

There is no attempt at named exports afaict:

otel/index.js

Line 410 in 1f850f6

module.exports = FastifyOtelInstrumentation

Copy link
Member

@mcollina mcollina Mar 18, 2025

Choose a reason for hiding this comment

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

@bcomnes can you add the named export support?

Copy link
Contributor Author

@bcomnes bcomnes Apr 6, 2025

Choose a reason for hiding this comment

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

I added named exports and a default export.

Copy link
Contributor Author

@bcomnes bcomnes Apr 6, 2025

Choose a reason for hiding this comment

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

I don't think it's possible to support named mjs exports correctly when assigning module.exports like this. MJS resolves this as { default, and 'module.exports' }.

// Declare the CommonJS export object
declare namespace FastifyOtelInstrumentation {
export {
FastifyOtelInstrumentation,
FastifyOtelInstrumentationOpts,
FastifyOtelOptions,
FastifyPlugin,
FastifyInstance
}
}

export = FastifyOtelInstrumentation
Copy link
Member

@Eomm Eomm Mar 17, 2025

Choose a reason for hiding this comment

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

Good spot!

Is this enough? Should we export the default as other plugins do?

https://github.com/fastify/fastify-redis/blob/713d5019c5bcc7003a0cf42c13cae56551074332/types/index.d.ts#L37-L41

Copy link
Contributor Author

@bcomnes bcomnes Mar 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't recall spotting other issues on other plugins, but also I import them into register in an fp so maybe I missed the issues. If it's an issue here, then it's also presumably an issue elsewhere. There might be other places where this is addressed, would need to investigate.

Copy link

@voxpelli voxpelli Mar 17, 2025

Choose a reason for hiding this comment

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

Should we add some tests using eg tsd?

Copy link
Member

@mcollina mcollina Mar 18, 2025

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor Author

@bcomnes bcomnes Apr 6, 2025

Choose a reason for hiding this comment

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

I mucked around with the tests, and I simply am not familiar enough with TSD or CJS/ESM nuance tests to know how to actually test for this.