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

bcomnes · 2025-03-17T04:56:22Z

Fixes #24

Fix default and named exports in mjs consumers by assigning module.exports and module.exports.FastifyOtelInstrumentation instead of adding static properties to the class. The properties show up as module.exports in mjs and named exports were not working before. Now they are importable in cjs and mjs as one would expect.
Added tests for mjs imports.
Convert index.d.ts to use export = because this is a type file for a cjs module so we cannot use export foo or export default anywhere without introducing a mismatch between the module system being used in the implementation and types.
Move the pure types into a type.d.ts file, where we can use named exports, since exporting them out of a cjs type file is very weird and awkward and requires typeof use by consumers.
Moved the fastify mocks from Types from fastify devDependency should not be imported in index.d.ts #32
to a fastify-mocks.d.ts file to also use named exports but to also discourage their import from consumers, just in case their editor tries to import a fastify type incorrectly out of this module. In general mocking these types seems like it's asking for trouble from a maintenance perspective. Fastify was previously an unlisted peer dependency (presumably because listed peer dependencies can be a nightmare) just as it is in nearly every other fastify plugin. Mocking these types will require ongoing one-off maintenance and will also potentially mask breaking issues as they materialize. Maybe someone more familiar with the deliberate strategy of using unlisted peer dependencies can chime in.

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

metcoder95

can you add some tests for it? Check test/index.test-d.ts

Eomm · 2025-03-17T16:57:22Z

index.d.ts

@@ -16,5 +16,4 @@ declare class FastifyOtelInstrumentation<Config extends FastifyOtelInstrumentati
  plugin (): (instance: FastifyInstance, opts: FastifyOtelOptions, done: (err?: Error) => void) => void
 }

-export default FastifyOtelInstrumentation
-export { FastifyOtelInstrumentation }
+export = FastifyOtelInstrumentation


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

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.

Should we add some tests using eg tsd?

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.

bcomnes · 2025-03-17T23:07:41Z

can you add some tests for it? Check test/index.test-d.ts

Yes, I will add some when I find time on an evening. Anyone feel free to beat me to it.

bcomnes · 2025-03-17T23:14:27Z

index.d.ts

@@ -16,5 +16,4 @@ declare class FastifyOtelInstrumentation<Config extends FastifyOtelInstrumentati
  plugin (): (instance: FastifyInstance, opts: FastifyOtelOptions, done: (err?: Error) => void) => void
 }

-export default FastifyOtelInstrumentation
-export { FastifyOtelInstrumentation }


There is no attempt at named exports afaict:

otel/index.js

Line 410 in 1f850f6

module.exports = FastifyOtelInstrumentation

@bcomnes can you add the named export support?

I added named exports and a default export.

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' }.

The typescript typings were exporting using the ESM style exports, but this is a CJS module, so the exports were not resolving correctly. This switches the type export to use `export = ` syntax instead which works for ESM consumers as well as CJS consumers. Fixes fastify#24 Signed-off-by: Bret Comnes <166301+bcomnes@users.noreply.github.com>

bcomnes · 2025-04-06T22:04:37Z

@mcollina, I don't think named exports are possible here. This module is assigning a class to module.exports with static fields, which works for simulated named exports in cjs const { FastifyOtelInstrumentation } = require('FastifyOtelInstrumentation') along side a cjs style default export const FastifyOtelInstrumentation = require('FastifyOtelInstrumentation'), but in mjs this just resolves to { default, 'module.exports' }, wherein only the default export works, and none of the desired named fields works.
The types in this module are broken for mjs consumers, because this is a cjs module exporting types as if it were an esm module. Let's fix that!
This situation is now worse since chore(types): Vendor in FastifyInstance type #35 has added additional esm style type exports.
I've updated the additional type exports as best as I understand, but in general, the circular references and almost named exports really confuses everything.

Any suggestions on how to test this?

bcomnes · 2025-04-06T22:50:03Z

I got the named exports working with the top level class. I'll send an alternative request in shortly.

bcomnes · 2025-04-07T01:35:29Z

Closing this PR out in favor of https://github.com/fastify/otel/pull/43/files

metcoder95 reviewed Mar 17, 2025

View reviewed changes

Eomm reviewed Mar 17, 2025

View reviewed changes

bcomnes commented Mar 17, 2025

View reviewed changes

bcomnes force-pushed the patch-1 branch from 17ba7b0 to 3f20786 Compare April 6, 2025 20:44

bcomnes force-pushed the patch-1 branch from 3f20786 to 1204156 Compare April 6, 2025 21:11

bcomnes mentioned this pull request Apr 7, 2025

fix(types): named and default exports and types in MJS #43

Open

4 tasks

bcomnes closed this Apr 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bcomnes commented Mar 17, 2025 •

edited

Loading

metcoder95 left a comment

Eomm Mar 17, 2025

bcomnes Mar 17, 2025 •

edited

Loading

voxpelli Mar 17, 2025

mcollina Mar 18, 2025

bcomnes Apr 6, 2025

bcomnes commented Mar 17, 2025

bcomnes Mar 17, 2025

mcollina Mar 18, 2025

bcomnes Apr 6, 2025

bcomnes Apr 6, 2025

bcomnes commented Apr 6, 2025

bcomnes commented Apr 6, 2025

bcomnes commented Apr 7, 2025

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

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

Conversation

bcomnes commented Mar 17, 2025 • edited Loading

Checklist

metcoder95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcomnes Mar 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcomnes commented Mar 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcomnes commented Apr 6, 2025

bcomnes commented Apr 6, 2025

bcomnes commented Apr 7, 2025

bcomnes commented Mar 17, 2025 •

edited

Loading

bcomnes Mar 17, 2025 •

edited

Loading