chore(gatsby): jaeger-local to TypeScript #23656
Conversation
gatsbot
bot
added
the
status: triage needed
pieh
added
status: needs core review
| @@ -0,0 +1,37 @@ | |||
| // eslint-disable-next-line @typescript-eslint/ban-ts-ignore | |||
| // @ts-ignore | |||
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I also don't have it installed:
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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.
|
I tested it out locally and |
chooban
force-pushed
the
ts-migration/jaeger-local
branch
from
44010e9 to
47adc64
Compare
|
I've updated the version of the types package, and merged up to date. @pieh, it involved a force push to my repo, so if you want to check it out locally you'll need to take that into account. |
|
Will do quick local test and merge this one if there are no problems (there shouldn't be as only thing that changed was removing few |
|
You're welcome! I've been working on another that's beyond my understanding, so I might put up a "help wanted" PR on the "help wanted" migration issue! |
Description
Migrates the Jaeger config file which is referenced in the performance tracing docs.
Since it's an example, the Jaeger client package itself is not in the dependencies.
Documentation
The file should be transpiled to JS so won't need any docs updating.
Related Issues
ref #21995