Add TypeScript type definitions (from DefinitelyTyped) #610
Conversation
benvinegar
force-pushed
the
add-ts-typings
branch
from
53fac2e to
dc03a30
Compare
|
Can we wire this up with tests so that this doesn't ever diverge from the actual API? I can see someone modifying one without the typescript definition very easily. |
|
So, you should note that this is already a problem today: if we change the Raven.js API, and users download the latest raven-js but nobody updates the But ... there is already a test file we can also borrow and run as part of CI. It will at least catch basic stuff and remind us that the definition file fundamentally may need updating. That work? |
Yeah, totally. The difference though is that this definition wasn't even maintained by us. So if we're now choosing to take on the burden of maintaining it correctly, we need to make sure we don't break it.
Seems reasonable, but I have significantly less context about what any of this is. :) I just want to make sure tests catch when we change the API and it breaks the ts definitions. |
If I add a new API method, it's going to require that I add a new rule, and an existing test won't catch this. The only possibility of automating this is by doing our own static analysis of public Raven methods (e.g. using Esprima) and verifying that a TypeScript definition exists for each. That is just way, way too much work for what we're getting out of this (right now). But, a test like the one I linked will at least catch definition file regressions. I think that is an acceptable place to start. |
|
@mattrobenolt – as always, thank you for pushing me to do this. Adding this test highlighted some issues and I think we're in a good place now. |
| id: '123' | ||
| }); | ||
| __1["default"].captureMessage('Broken!'); | ||
| __1["default"].captureMessage('Broken!', { tags: { key: "value" } }); |
| ignoreErrors?: string[]; | ||
|
|
||
| /** Similar to ignoreErrors, but will ignore errors from whole urls patching a regex pattern. */ | ||
| ignoreUrls?: RegExp[]; |
There was a problem hiding this comment.
It seems that these don't allow passing strings as well as regexp.
|
|
||
| /** Similar to ignoreErrors, but will ignore errors from whole urls patching a regex pattern. */ | ||
| ignoreUrls?: RegExp[]; | ||
| ignoreUrls?: RegExp[] | string[]; |
There was a problem hiding this comment.
This still doesn't cover the case of:
ignoreUrls: [
/foo/,
'bar',
]Does it?
There was a problem hiding this comment.
Ugh you're right, just tested it
There was a problem hiding this comment.
Fixed. We're learning TypeScript!
Greatly facilitates importing Raven.js into a TypeScript project (otherwise you must use an external definition file). Most other major crash reporting platforms embed a definition file in their JS client repository.
This definition file was taken from the DefinitelyType repository, with just a single change: I made
Raventhe default export for the module, so that this is the same:One thing that isn't clear to me, but we can resolve in a later patch, is whether we need the duplicated API documentation / JSDoc strings. I guess one nice thing is that these comments appear in Visual Studio and other tools.
cc @mattrobenolt @santialbo @spartan563