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

Can the runtime dependency on types be moved to devDependencies #1787

Closed
lilwilsond2 opened this issue Nov 9, 2023 · 4 comments · Fixed by #1804 or #1829
Closed

Can the runtime dependency on types be moved to devDependencies #1787

lilwilsond2 opened this issue Nov 9, 2023 · 4 comments · Fixed by #1804 or #1829
Assignees
Labels
bug Something isn't working pkg:instrumentation-express priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies

Comments

@lilwilsond2
Copy link

What version of OpenTelemetry are you using?

1.6.0

What version of Node are you using?

18.17.0

What did you do?

The inclusion of @types/express as a runtime dependency causes some typing issues for us in mocked tests.
I found this discussion about why the types were moved originally, but i was wondering if the types could be moved back at this point

What did you expect to see?

type defs as a dev dependency

What did you see instead?

type defs as a runtime dependency

Additional context

fwiw, i added this to my package json and things still seem okay

"resolutions": {
    "@types/express": "link:./node_modules/.cache/null"
  },
@lilwilsond2 lilwilsond2 added the bug Something isn't working label Nov 9, 2023
@Flarna
Copy link
Member

Flarna commented Nov 10, 2023

The problem is that ExpressInstrumentation has types from express in it's public interface.
e.g. the requestHook uses type Request.

If some typescript user installs the instrumentation (maybe indirect via autoinstrumentations meta package) but not the express types typescript complains.

Therefore to move it into dev-deps it's additionally needed to cleanup the public API surface as described here.

@david-luna
Copy link
Contributor

@Flarna

The description of requestHook says

Function for adding custom attributes on Express request.

Could we limit the request type to a subset of props (the ones necessary to add attributes) and allowing the user to pass the full type if needed?

export interface SimpleExpressRequest {
  // some props here
}

export type ExpressRequestInfo<T = SimpleExpressRequest> = {
  request: T;
  route: string;
  layerType: ExpressLayerType;
};

@Flarna
Copy link
Member

Flarna commented Nov 15, 2023

As long as the dependency to express is broken it should be fine. The instrumentation itself doesn't need the hooks therefore I think no need for a SimpleExpressRequest types. We could default to unknown or any.

But for sure the docs need to mention what is actually given to the hook.

@pichlermarc pichlermarc added priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies pkg:instrumentation-express labels Nov 15, 2023
@david-luna
Copy link
Contributor

@Flarna @pichlermarc I can prepare a PR. you can assign it to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pkg:instrumentation-express priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies
Projects
None yet
4 participants