search

newrelic / newrelic-winston-logenricher-node

This is no longer maintained. The work has been moved to https://github.com/newrelic/newrelic-node-log-extensions/tree/main/packages/winston-log-enricher
Apache License 2.0
6 stars 8 forks source link

add rudimentary typescript types #60

Closed garbados closed 3 years ago

garbados commented 3 years ago

Proposed Release Notes

Links

Details

This PR adds TypeScript types to the log enricher. As of this writing, these types are all instances of any and thus not particularly useful. I'll narrow it down before I submit it for review.

CLAassistant commented 3 years ago

CLA assistant check
All committers have signed the CLA.

garbados commented 3 years ago

It is EOD for me so I am leaving some links here to help narrow down the types we expose, so that they can be more useful to downstream TypeScript users.

More coming soon!

garbados commented 3 years ago

Ready to go, my dudes 😎

garbados commented 3 years ago

It also might be helpful to include in the details how to test this.

So for testing, it seems we have two options: tsd and Microsoft's dtslint. Both involve writing additional tests. Based on what I've seen, I'd opt for using tsd. I can write these tests tomorrow.

I'm not that familiar with ts projects but I think you need to include a files list of type definitions in your project so the ts compiler can parse them, no?

I can't seem to find documentation indicating this. The file structure in this PR was auto-generated by the TypeScript CLI. I'm sure the details of this will shake out as I instrument testing.

garbados commented 3 years ago

I can't seem to find documentation indicating this.

Nevermind. Found it.

garbados commented 3 years ago

I've updated the type definitions and the package meta files to include type testing in the test suite, though I am still experiencing this error:

$ npx tsd

  types/index.test-d.ts:5:0
  ✖  5:0  Parameter type typeof winston is not identical to argument type any.  

  1 error

Being largely unfamiliar with Typescript, I'm kind of fumbling around in the dark. I've exhausted my threads for now so any guidance would be appreciated. I'm going to freshen up my brain and try again.

garbados commented 3 years ago

OK, thanks to help from @michaelgoin I got the type definitions to a point where they compile appropriately. You can test this yourself by npm installing this branch and then running a TS file like this:

import * as logEnricher from '@newrelic/awinston-enricher'

console.log(logEnricher)

It won't fail, which gives us pretty much all the info we need.

tsd gave us enough problems that I have opted to not use it, and I haven't tried its alternative dtslint yet. I'll do that tomorrow. But what we have now seems good enough to use, so what remains is just to instrument testing.

garbados commented 3 years ago

Ready for review!

michaelgoin commented 3 years ago

Appears to be failing to load typescript in the CI. Perhaps it by default is trying to load a global install?

garbados commented 3 years ago 
$ npm i -g typescript && npm run types
...
Error: Cannot find module 'typescript'

:/

garbados commented 3 years ago

I can't approve because I'm the original author, but this LGTM. Thanks @michaelgoin !