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.

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

chore(gatsby): jaeger-local to TypeScript #23656

Merged
merged 4 commits into from
May 5, 2020
Merged

chore(gatsby): jaeger-local to TypeScript #23656

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3a3223c
chore(gatsby): jaeger-local to TypeScript
chooban Apr 30, 2020
d5ebb4e
Add interface extension and reason
chooban May 4, 2020
ae04f78
Remove apparently unused ts-ignore, inlined interface extension
chooban May 4, 2020
47adc64
Upgrade types package, and remove interface extension
chooban May 5, 2020
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
Prev Previous commit
Next Next commit
Add interface extension and reason
  • Loading branch information
@chooban
chooban committed May 5, 2020
commit d5ebb4e5e49d00be76313ebc7a5994330d205208
13 changes: 8 additions & 5 deletions packages/gatsby/src/utils/tracer/jaeger-local.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,14 @@
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Why there is // @ts-ignore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies for the lack of comment. This is because we don't have a dependency on jaeger-client as this is an example to be used in other projects. No dependency means we get the Cannot find module error.

Thinking about how the file is to be used, it might be a candidate for a @ts-nocheck, but if we do migrate it, and types are added, at least we'll get warnings if a jaeger change breaks it. I am now arguing myself in circles!

Copy link
Contributor

Choose a reason for hiding this comment

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

We could try making jaeger-client optional peer dependency ( https://medium.com/@noamgjacobsonknzi/what-is-peerdependenciesmeta-acea887c32dd )

This is supported by yarn@^1.13.0 and npm@^6.11.0 - worry here is that npm@^6.11.0 started shipping with Node.js in Node.js 12.11.0, so there would be quite a bit of not resolver peer deps warnings if user didn't upgrade npm if they are on earlier node versions

But also - I removed this @ts-ignore locally for testing and didn't see complaints when trying eslint and typecheck - what am I missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have jaeger-client installed at all? If I do yarn why jaeger-client I get the We couldn't find a match that's expected, then yarn typecheck throws an error when I run it. I also get the warning in VS Code, but it's probably safer to just check the official scripts.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also don't have it installed:
Screenshot 2020-05-04 at 15 22 25

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am now in the same position as you! I noticed the last push failed typecheck on CI, which surprised me as there'd been no issues locally, so I did yarn bootstrap; yarn typecheck just to make sure I wasn't missing anything, and the CI error appeared. Fixed it, removed the ts-ignore in question, and now everything is perfectly happy.

I wish I had a longer history set in my terminal so that I could screenshot it!

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's see what CI says after last commit you pushed. There just be more to it (or maybe it wasn't typecheck that failed?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The last CI failure was typecheck, but was to do with the extended interface I added. Because initTracer returns a JaegerTracer object, it was complaining that it didn't match my new interface. Obvious, really, but for some reason I didn't get the failure in VS Code. The lint-staged task doesn't trigger on changes to TypeScript files so it wasn't caught by husky.

import { initTracer, JaegerTracer, TracingConfig } from "jaeger-client"

let tracer: JaegerTracer
// The close method is currently in review at DefinitelyTyped:
// https://github.com/DefinitelyTyped/DefinitelyTyped/pull/44132
// Once that's merged, we can remove this manual extension and use the base version.
interface IJaegerTracerWithCLose extends JaegerTracer {
close(callback?: () => void): void
}

let tracer: IJaegerTracerWithCLose

function create(): JaegerTracer {
// See schema
Expand All @@ -26,10 +33,6 @@ function create(): JaegerTracer {

function stop(): Promise<void> {
return new Promise(resolve => {
// It's not on the @types definition, but the Jaeger documentation tells us to call
// close(): https://github.com/jaegertracing/jaeger-client-node#trace-buffer
// eslint-disable-next-line @typescript-eslint/ban-ts-ignore
// @ts-ignore
tracer.close(resolve)
})
}
Expand Down