thrift-server: (extra-large) update dependencies, rename @creditkarma -> @statestitle #5
Conversation
strmer15
changed the title
strmer15
changed the title
| }, | ||
| "devDependencies": { | ||
| "lerna": "^3.14.1" | ||
| "lerna": "^4.0.0" |
There was a problem hiding this comment.
I'm guessing no breaking changes we care about
There was a problem hiding this comment.
Here's the changelog: https://github.com/lerna/lerna/blob/main/CHANGELOG.md
It didn't seem like anything that I was too worried about, a lot of exports but I don't think this code uses lerna programmatically, and the only other thing was dropping Node 6 and 8 support.
There was a problem hiding this comment.
tests passed for me!
there are some creditkarma things in README but they don't really matter:
$ git grep credit.*thrift-client
README.md:} from '@creditkaram/thrift-client'
README.md:} from '@creditkaram/thrift-client'
packages/thrift-client-zipkin-filter/README.md:} from '@creditkaram/thrift-client'
🎉
ooh, thanks - I'll fix those, I think at first I was skipping the README files and then I went back to update them too, but obviously missed some. |
| @@ -177,10 +177,14 @@ export class Connection { | |||
| } | |||
| } | |||
| } catch (err) { | |||
| let message = '' | |||
There was a problem hiding this comment.
I'm wondering if there's a better pattern we should use here. Like use "Unknown" if it's not an instanceof Error? Or err = message?
There was a problem hiding this comment.
Sure, I'll make the default message be Unknown, seems like a decent idea.
| code: { status: new thrift.Int64(0) }, | ||
| value: 'test', | ||
| }, | ||
| { skip: ['_annotations', '_fieldAnnotations'] }, |
There was a problem hiding this comment.
Why do we need to skip comparing these fields now? (in this file and in others in the PR)
There was a problem hiding this comment.
There were some changes in the @hapi/code library (test matchers, https://github.com/hapijs/code) that seem to make this not work the way it was before - specifically object comparisons where one is missing keys that the other has. I think that it may have been caused by this change?
Anyways, the tests were failing because those two keys were present in the resulting object but not in the object here in the test. I couldn't figure out a better way to do this other than skipping the problematic properties (the includes function didn't seem to do it). In Jest I'd just use expect.objectContaining but I couldn't find similar functionality in @hapi/code.
| @@ -47,7 +47,7 @@ function is128bit(traceId: TraceId): boolean { | |||
| export function serializeLinkerdHeader(traceId: TraceId): string { | |||
| const serialized: ByteBuffer = ByteBuffer.concat([ | |||
| ByteBuffer.fromHex(traceId.spanId), | |||
| ByteBuffer.fromHex(traceId.parentId), | |||
| ByteBuffer.fromHex(traceId.parentSpanId.getOrElse('')), | |||
There was a problem hiding this comment.
I think I don't understand the getOrElse() API. It seems like it makes things hard to use. Can parentSpanId ever actually be unset? Is it really safe to use an empty string in those cases?
I'm obviously not familiar with any of the code in this file.
There was a problem hiding this comment.
Yeah, the zipkin library updates (used for request tracing, it seems) changed the name but also wrapped these fields in some kind of Optional object where you have to call this method or something similar to unwrap the actual value (it's similar to other Java APIs I've seen in the past for wrapping possible null values). You can call getOrElse() without a default value and it would be string | undefined, but the fromHex function requires a string, so this seemed like the closest equivalent to how it would have been before? All the tests pass and this seems like the best equivalent - apparently parentSpanId can be unset (otherwise it wouldn't be this Optional type), and I believe an empty string is pretty safe to use (certainly more so than a null or undefined value).
I'm not crazy about the syntax either (obviously a ?? would seem a bit nicer than calling getOrElse) but I also don't know how to really get that changed other than switching it to a different library. 🤷
Thanks for reviewing them! |
All the dependency versions have been updated to their latest, some notable things:
typescript- set to~4.4.4since the 4.5.x line has some performance issues and breaks thrift code generation due to some changed APIsnew Promise(...)needing to be changed tonew Promise<void>(...)in a few places, as well as needing to doinstanceofchecks on caught errors since they're now typed asunknownby default@types/node- set to^12.20.41to support Node 12 and up, since they're the current LTS versionsBufferAPIs that no longer allowed a boolean parameter for turning assertions on/offtslint- set to the highest version available, but the entiretslintpackage is deprecated in favor ofeslintand@typescript-eslint. This PR is big enough as it is without trying to also fix that.prettierupgrade, there were a bunch of linting fixes in this PRzipkin- this upgrade involved renamingparentIdtoparentSpanId, but also unwrapping some of the optional values usinggetOrElseAlso, all the
@creditkarmainstances have been renamed to@statestitlefor consistency, and Github links were fixed to point at the StatesTitle repo.@statestitle/typescript-thriftlibrary, which means that exceptions can't use themessagefield without it being required. Also, some tests were expecting to get a specificmessagestring, so the thrift files were updated to add the exception to thethrowsclause and set the message correctly in the server code.All tests pass now, but to try this out you'll need to run
npm run clean,npm install, thennpm test.