polish raven.d.ts #827

LucaVazz · 2017-01-12T23:03:44Z

as discussed in #820

added missing public functions from raven.js
moved complex data types into helper interfaces
corrected wrong definitions
reordered things a bit

Please double-check that everything has the correct data types, according to the JS-Side.

- added missing public functions from raven.js - moved complex data types into helper interfaces - corrected wrong definitions - reordered things a bit

benvinegar · 2017-01-17T18:51:04Z

cc @connor4312 – mind chiming in on this?

connor4312 · 2017-01-17T18:56:30Z

typescript/raven.d.ts

+     *
+     * @param user The definition of the currently active user's unique identity
+     */
+    setUserContext(user: RavenUserContext): RavenStatic;


You can remove the overload and just have user?: RavenUserContext

But if the overload is removed, the difference in the documentation is lost.

As the function behaves (a bit) differently, if you pass it nothing or a RavenUserContext, I wanted to also reflect that in the inline-/tooltip -documentation

connor4312 · 2017-01-17T18:57:36Z

typescript/raven.d.ts

+     * Specify a callback function that determines if the given message should be sent to Sentry.
+     * @param callback The function which determines if the given blob should be sent
+     */
+    setShouldSendCallback(callback: (data: any, orig?: string) => boolean): RavenStatic;


That's a lot of anys! Can we get some more specific typings on them?

It sure is...
I will look into it again if it is (reasonably) possible to be more specific about the data passed to these various callbacks

connor4312 · 2017-01-17T18:59:27Z

Looks mostly fine as far as the typings themselves go, I've not verified that all data types are correct against the Js implementation.

benvinegar · 2017-01-20T01:28:54Z

Okay, CI isn't running npm test (will fix), otherwise we'd see:

> raven-js@3.9.1 test-typescript /Users/benvinegar/Projects/raven-js
> node_modules/typescript/bin/tsc --noEmit --noImplicitAny typescript/raven-tests.ts

typescript/raven-tests.ts(1,21): error TS2307: Cannot find module 'raven-js'.

So, this is the problem I have every time I try to touch our module definition: I cannot come up with a consistent export that works wherever.

Can someone here help me? Am I invoking tsc incorrectly?

benvinegar · 2017-01-20T01:29:40Z

Also, why rename to RavenJS? I'd kind of prefer if it was just Raven.

LucaVazz · 2017-01-20T08:04:15Z

If I remember it correctly, a tsconfig.json is needed in ./typescript to successfully run tsc; somehow the file got lost...
I will add it later.
As far as I can tell you are invoking tsc correctly nonetheless.

RavenJS is only a local name inside raven-tests.ts, so it should be possible to change it without a problem.

benvinegar · 2017-01-20T18:21:03Z

@LucaVazz – again, really appreciate your continued effort on this. I've re-enabled the tsc test in CI to help facilitate.

…in before-previous commit)

LucaVazz · 2017-01-24T01:44:31Z

I took me a little longer then I expected to get around to doing this...

Now tsc works without errors and I made any wherever I could more specific.

# Conflicts: # typescript/raven.d.ts

benvinegar

See comments: really just questions.

benvinegar · 2017-01-25T00:36:19Z

typescript/raven-tests.ts

-Raven.setEnvironment('production');
-
-Raven.setDataCallback(function (data: any) {});
-Raven.setDataCallback(function (data: any, original: any) {});


Why remove these from tests?

That was not intentional.
As to why it did happen... Good question.

fixed in 1f55f01

benvinegar · 2017-01-25T00:37:20Z

typescript/raven.d.ts

+
+
+// --- Helper Interfaces for Options --------------
+export interface RavenBreadcrumOptions {


Typo: Breadcrum

fixed in 3e51fc6

benvinegar · 2017-01-25T00:37:51Z

typescript/raven.d.ts

+     colno: string;
+     function:string;
+     in_app: boolean;
+}


At 387 lines, this definition file is almost as complicated as Raven.js itself. Is it normal for libraries to expose something so rigid? I'm terrified this file will become really painful to maintain.

Well, I agree with what @connor4312 previously said: More specific type definitions are always better.
Unfortunately I don't see a way to compress the type definitions for Raven more...

One way to ensure a bit easier maintainability would be to switch the main Raven-JS to TypeScript. It wouldn't be a change that drastically, especially considering that TypeScript transpiles down to vanilla JS in the end.

One way to ensure a bit easier maintainability would be to switch the main Raven-JS to TypeScript. It wouldn't be a change that drastically, especially considering that TypeScript transpiles down to vanilla JS in the end.

This will not happen :)

okay then ;)

But still, I don't think that it would be that hard to maintain this file onwards. In my opinion it is "just" another documentation of the Raven Module, like a more specific version of JSDoc which you already use.
Maybe @connor4312 has some additional opinion...?

Yea, so. I would mention that although the length may be comparable to raven.js itself, the complexity is significantly lower; it's static definitions, not procedural code.

That said, it is a maintenance burden, and it's definitely your prerogative whether that's something you want to take up. Someone will have to take it up, if nothing else maintaining definitions in DefinitelyTyped. It could be reduced by making pull request templates and having an issue checkmark for "[x] I updated typings/raven.d.ts if I added to or changed the API". I'd be happy to assist with reviews for those if you'd like.

I think that is a really good idea @connor4312!

I have added a Proposal for a PR-Template which includes that. What do you think of it @benvinegar?

If we're to add such a template, I'd rather it not be part of this PR.

I see, then I will try that in a separate Request.

benvinegar · 2017-01-25T00:39:08Z

typescript/raven.d.ts

+
+export interface RavenBreadcrumb {
+    message: string;
+    data: { [id: string]: string };


Will this allow for free-form objects?

Regarding the RavenUserContext or RavenBreadcrumb?

benvinegar · 2017-01-25T00:43:40Z

typescript/tsconfig.json

@@ -0,0 +1,23 @@
+{
+    "compilerOptions": {
+        "module": "commonjs",


What can we do to guarantee that users, no matter what environment, will be able to import this project fine? Do we have to document, "please see our tsconfig settings"?

Would you expect the export/module TypeScript definition change to result in a new major rev (4.x.x)?

No, this tsconfig settings only define the structure and compilerOptions for this module. Users should be able to import the module fine as long as our type definitions are correct, regardless if their project's tsconfig settings match these or not.

In my opinion this is still a non-breaking change, as long as users used the interface correctly. I didn't change anything major at the tests (as I said, the Deletion was unintentional) and they still pass.

benvinegar · 2017-01-26T00:06:09Z

Okay, so the blocker for me right now has little to do with this PR (sort of).

It's that if you try to do this in a SystemJS project:

import Raven from 'raven-js';

SystemJS will look for .default on the imported Raven instance, and none exists (because we're not building one that way in our Grunt build).

That's why, up until now, we've advocated doing:

var Raven = require('raven-js');

Which does not look for a default property and works. It looks like I can still use this require statement even with the export changes you've done in this definition file. Can anyone confirm?

connor4312 · 2017-01-31T23:31:39Z

.github/PULL_REQUEST_TEMPLATE.md

+***Release Type:***
+- [ ] Major
+- [ ] Minor
+- [X] Patch


Please remove this.

connor4312 · 2017-01-31T23:32:52Z

.github/PULL_REQUEST_TEMPLATE.md

+***Checklist:***
+- [ ] I have included appropriate Tests and have run `npm test` without errors
+- [ ] I have updated the Documentation accordingly
+- [ ] I updated typings/raven.d.ts if I added to or changed the API and have run `tsc --noEmit --noImplicitAny typescript/raven-tests.ts` without errors


We can probably make this a script like npm run test:ts, or make it called by npm test directly

I just noticed that in fact it already is called directly by npm test 😁

LucaVazz · 2017-01-31T23:34:17Z

Regarding SystemJS: On the one hand I don't see a reason why the way you outlined wouldn't work anymore.
On the other hand I'm unfortunately not really experienced with SystemJS so someone else should better verify it....

see DefinitelyTyped/DefinitelyTyped#14674

see https://github.com/DefinitelyTyped/DefinitelyTyped/pull/14674/files

LucaVazz · 2017-02-16T14:05:57Z

the last two commits integrated changes suggested by @evilj0e in DefinitelyTyped/DefinitelyTyped#14674

benvinegar

Please remove PULL_REQUEST_TEMPLATE (see other comment) and I'll go ahead and merge.

benvinegar · 2017-02-17T21:42:10Z

typescript/raven.d.ts

+     colno: string;
+     function:string;
+     in_app: boolean;
+}


If we're to add such a template, I'd rather it not be part of this PR.

benvinegar · 2017-02-28T21:22:04Z

Haven't forgotten about this. I'll merge it for the next minor release (3.13.0).

This reverts commit 4e12341.

LucaVazz added 4 commits January 12, 2017 23:53

polished raven.d.ts

ef30661

- added missing public functions from raven.js - moved complex data types into helper interfaces - corrected wrong definitions - reordered things a bit

updated author information of raven.d.ts

98a83c1

added extra to RavenUserContext

2e590d2

small fixes

cc9fc47

LucaVazz mentioned this pull request Jan 12, 2017

updated Definitions for Raven-JS DefinitelyTyped/DefinitelyTyped#13978

Merged

7 tasks

copied fixes from PR in DefenitivelyTyped

592e542

connor4312 reviewed Jan 17, 2017

View reviewed changes

LucaVazz added 8 commits January 24, 2017 00:23

fixed tsc --noEmit --noImplicitAny typescript/raven-tests.ts

089efdd

made data: any in set...Calback more specific

305d38b

added missing values to RavenOutgoingData

949bc5d

replaced any in setBreadcrumbCallback

d0d32c8

made options for setTransport more specific

e562d5c

set user in RavenOutgoingData to RavenUserContext instead of any

a53a044

fixed missed RavenTransportOptions (-> RavenTransportFunctionOptions …

f46fde8

…in before-previous commit)

fixed interface-props terminated by ; to be terminated by ;

7fdef1f

Merge branch 'master' of https://github.com/getsentry/raven-js

9f38a30

# Conflicts: # typescript/raven.d.ts

benvinegar suggested changes Jan 25, 2017

View reviewed changes

LucaVazz added 3 commits February 1, 2017 00:16

re-added accidentally deleted tests (in a fixed version)

1f55f01

fixed typo in RavenBreadcrumbOptions

3e51fc6

added a proposal for a PR-Template

ac60fe3

connor4312 reviewed Jan 31, 2017

View reviewed changes

LucaVazz added 3 commits February 1, 2017 00:39

removed redundant testing from the PR-Template

10d48cc

fix optionals in RavenBreadcrumb

60a7809

see DefinitelyTyped/DefinitelyTyped#14674

added tests for captureBreadcrumb

5e92191

see https://github.com/DefinitelyTyped/DefinitelyTyped/pull/14674/files

fixed erroneous optionals from the last commit

634908c

connor4312 approved these changes Feb 16, 2017

View reviewed changes

benvinegar suggested changes Feb 17, 2017

View reviewed changes

LucaVazz added 2 commits February 19, 2017 22:28

removed PR-Template

d8941cf

Merge branch 'master' of https://github.com/LucaVazz/raven-js

e034ba6

This was referenced Feb 19, 2017

Change to proper statement of single class export for typescript. #854

Closed

Add a Pull Request template #859

Closed

benvinegar approved these changes Mar 20, 2017

View reviewed changes

benvinegar merged commit 4e12341 into getsentry:master Mar 20, 2017

benvinegar mentioned this pull request Mar 22, 2017

Typing errors on 3.13.0 release #898

Closed

benvinegar added a commit that referenced this pull request Mar 22, 2017

Revert "polish raven.d.ts (#827)"

f55d281

This reverts commit 4e12341.

LucaVazz mentioned this pull request Jul 7, 2017

types are exported as a module #977

Merged



		// --- Helper Interfaces for Options --------------
		export interface RavenBreadcrumOptions {

polish raven.d.ts #827

polish raven.d.ts #827

Conversation

LucaVazz commented Jan 12, 2017

benvinegar commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connor4312 commented Jan 17, 2017

benvinegar commented Jan 20, 2017 • edited Loading

benvinegar commented Jan 20, 2017

LucaVazz commented Jan 20, 2017

benvinegar commented Jan 20, 2017

LucaVazz commented Jan 24, 2017

benvinegar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucaVazz Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

benvinegar commented Jan 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucaVazz commented Jan 31, 2017

LucaVazz commented Feb 16, 2017 • edited Loading

benvinegar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benvinegar commented Feb 28, 2017

benvinegar commented Jan 20, 2017 •

edited

Loading

LucaVazz Jan 25, 2017 •

edited

Loading

LucaVazz commented Feb 16, 2017 •

edited

Loading