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

fix: Use real fastify request type #1753

Closed
wants to merge 2 commits into from

Conversation

asakusuma
Copy link

Replace the any with the real type. I tested this locally in a node app.

Which problem is this PR solving?

The request object coming from the requestHook is of type any, so you don't get autocompletion of fields on the request object.

Short description of the changes

Import the type from fastify package.

Replace the `any` with the real type. I tested this locally in a node app.
@asakusuma asakusuma requested a review from a team October 26, 2023 17:14
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 26, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@github-actions github-actions bot requested a review from pichlermarc October 26, 2023 17:14
@asakusuma asakusuma changed the title Use real fastify request type Fix: Use real fastify request type Oct 26, 2023
@asakusuma asakusuma changed the title Fix: Use real fastify request type fix: Use real fastify request type Oct 26, 2023
@Flarna
Copy link
Member

Flarna commented Oct 30, 2023

This change is problematic.
Using fastify types in public interface requires that end users (using typescript) have fastify types installed. Fastify types are part of fastify itself so this would require that all users have fastify installed.
In special users for users of @opentelemetry/auto-instrumentations-node this is often not the case.

So it would be needed to add fastify as a dependency here which is not wanted because the instrumentation should not depend on the module to instrument.

See GUIDELINES for more details.

Copy link
Member

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @asakusuma

I assume you tested it with a setup that includes fastify in the node_modules.

The types.ts file is part of the package public API, since it is exported from index.ts.
Thus, any typescript program that will import the instrumentation will hit this line and will attempt to

import type { FastifyRequest } from 'fastify';

If your setup does not include the fastify package, typescript will fail transpilation, which is the reason why this object is typed with any in the first place.

You can read more info here.

@blumamir
Copy link
Member

blumamir commented Nov 8, 2023

This PR cannot be merged as is due to the issues mentioned above.
If you think this is a mistake, please feel free to re-open.

Thanks!

@blumamir blumamir closed this Nov 8, 2023
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 this pull request may close these issues.

4 participants