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

feat: add TypeScript types #67

Merged
merged 4 commits into from
Apr 19, 2023
Merged

feat: add TypeScript types #67

merged 4 commits into from
Apr 19, 2023

Conversation

trentm
Copy link
Member

@trentm trentm commented Apr 13, 2023

These should be considered experimental. I'm not confident in my
TypeScript typing.

These are based on the types from @opentelemetry/instrumentation: https://github.com/open-telemetry/opentelemetry-js/blob/v1.11.0/experimental/packages/opentelemetry-instrumentation/src/platform/node/require-in-the-middle.d.ts

checklist

  • Sanity check with some of the OTel JS maintainers. The goal here is to potentially replace the need for the above "require-in-the-middle.d.ts" file in @opentelemetry/instrumentation.
  • Perhaps add a test case here for the types.

These should be considered experimental. I'm not confident in my
TypeScript typing.
@trentm
Copy link
Member Author

trentm commented Apr 13, 2023

open-telemetry/opentelemetry-js#3727 is my draft PR that would use these types. One possible concern are some of these linter warnings in the OTel repo:

[opentelemetry-js2/experimental/packages/opentelemetry-instrumentation]% npm run lint

> @opentelemetry/instrumentation@0.37.0 lint
> eslint . --ext .ts


/Users/trentm/tm/opentelemetry-js2/experimental/packages/opentelemetry-instrumentation/src/instrumentation.ts
  34:51  warning  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any

/Users/trentm/tm/opentelemetry-js2/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentation.ts
   33:47  warning  Unexpected any. Specify a different type    @typescript-eslint/no-explicit-any
  100:22  warning  'T' is already declared in the upper scope  @typescript-eslint/no-shadow

/Users/trentm/tm/opentelemetry-js2/experimental/packages/opentelemetry-instrumentation/src/platform/node/instrumentationNodeModuleDefinition.ts
  31:39  warning  Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any

✖ 4 problems (0 errors, 4 warnings)

For example, is that 100:22 warning 'T' is already declared in the upper scope @typescript-eslint/no-shadow warning an issue because I used T in:

export type OnRequireFn = <T>(exports: T, name: string, basedir?: string) => T;

?

@david-luna
Copy link
Member

david-luna commented Apr 17, 2023

Hi Trent,

I've did a clean checkout of OTel repo and I did have the same warnings so the warnings you see are scoped in the printed files and your typings in require-in-the-middle do not affect the linting in any way. That's good :)

So what about these warnings?

The linter is mainly warning about the code using the type any to define some generic types. The recommendation is to use unknown instead so TS compiler is stricter when you're manipulating objects. IMHO fixing these warnings is out of scope.

For example, is that 100:22 warning 'T' is already declared in the upper scope @typescript-eslint/no-shadow warning an issue because...

Actually the TS compiler is complaining about a redundancy in generic types in the class definition. The class is defining a generic type in class type and also in a private method

export abstract class InstrumentationBase<T = any>
  extends InstrumentationAbstract
  implements types.Instrumentation
{
  // Some attributes & methods

  private _onRequire<T>(
    module: InstrumentationModuleDefinition<T>,
    exports: T,
    name: string,
    baseDir?: string
  ): T {
    // Implementation
  }
}

As you see we have a generic in the class definition InstrumentationBase<T = any> but also we have another definition in _onRequire<T> method. This is ambiguous and by their definitions we cannot tell if they refer to the same type or not. By looking at how it is used internally in here I think they're not the same because it is explicitly saying we want the same type that the exports object has.

If my observation is right just changing the generic name would be enough, something like the following would resolve the shadowing linter warning

  private _onRequire<ExportsType>(
    module: InstrumentationModuleDefinition<ExportsType>,
    exports: ExportsType,
    name: string,
    baseDir?: string
  ): ExportsType {
    // Implementation
  }

Also IMHO out of scope of this task but we can be good boy scouts and ask the author of the code

@trentm
Copy link
Member Author

trentm commented Apr 18, 2023

@david-luna Thanks for the review! I'll take open-telemetry/opentelemetry-js#3727 out of draft and give a chance for reviews there before merging this and doing another release with it.

@trentm trentm merged commit f50ac96 into main Apr 19, 2023
15 checks passed
@trentm trentm deleted the trentm/types branch April 19, 2023 17:27
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.

None yet

2 participants