Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

types are exported as a module #977

Merged
merged 1 commit into from
Jul 7, 2017

Conversation

polco
Copy link
Contributor

@polco polco commented Jun 13, 2017

Fixes #898. It doesn't change the current behavior but allow to access the internal type as you can see in the updated test file.

import * as Raven from 'raven'

function logMessage(message: string, level: Raven.LogLevel, ...) {
...
}

I tested with the typescript 1.8.10 referenced in the package.json and the latest one 2.3.4.
It kinda duplicates #736, but there was no movement for months now...

@polco
Copy link
Contributor Author

polco commented Jun 13, 2017

(the indentation makes the diff difficult to read but nothing was changed :) )

@MaxBittker
Copy link
Contributor

Hi @polco - as you may have noticed it's been hard for us to review type annotation improvements because we don't internally use this ecosystem and there seem to be many permutations on the ways people import modules and build their bundles. I appreciate this patch and want to help get it merged but definitely need to try it out on my own first and get some feedback from people using the types already - so bear with me.
I see that this makes it possible to import the internal interfaces which is great, what other effects does the patch have?

@polco polco force-pushed the typescript-module branch from 87a3608 to 7822eed Compare June 14, 2017 00:39
@polco
Copy link
Contributor Author

polco commented Jun 14, 2017

hello @MaxBittker i understand the hassle no problem. The patch doesn't change anything about the way raven-js was being used by typescript in the past versions and add nothing more than the access to the internal interfaces.
I actually was 'forced' into this PR by the latest version of typescript which strengthened their type inference system.

@MaxBittker
Copy link
Contributor

Hi polco - just wanted to say that I haven't forgotten about this but have been busy. 😬

@polco
Copy link
Contributor Author

polco commented Jun 24, 2017

Haha thank you ;)

@whatisaphone
Copy link

whatisaphone commented Jul 6, 2017

Hi, I'm the author of #736 from last year. Our company uses typescript extensively. We have a project that is currently stuck on raven-js@3.2.0. Today I tried this swapping in this PR via git+https://github.com/polco/raven-js.git#7822eed5d747ab073d20017bc335cd06de73a827. I verified that no existing code breaks, and that it restores the ability to access the types (using the same code we currently use on 3.2.0). It also updates the tests, which is an improvement over my quick and dirty PR.

You can see a simplified diff here that highlights what the real changes are. The approach taken by this PR is the same approach I've seen taken for other libraries that support being used both via a global and a module, such as sinon and lodash. (module and namespace mean the same thing in typescript.)

As far as I'm aware, exposing the types in this manner has no negative effects, and will not cause any changes for existing users of the library. Is there anything I can do to help get this merged?

Copy link
Contributor

@MaxBittker MaxBittker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnsoft - I really appreciate those links and your review here. I'm approving this PR and will merge + cut a new version on monday. @polco thanks again for the patch!

@MaxBittker MaxBittker merged commit da8503d into getsentry:master Jul 7, 2017
@LucaVazz
Copy link
Contributor

LucaVazz commented Jul 7, 2017

@polco @johnsoft This should make it possible to re-merge #827 to get the extended typings, right? Could you please verify that?

@whatisaphone
Copy link

@MaxBittker
No problem, glad to help!

@LucaVazz
That PR contains export default Raven; which is incorrect. There are two ways of declaring a default export in JS:

  1. Method one, CommonJS:
  • CommonJS export – module.exports = <object>
  • CommonJS import – var Raven = require('raven-js')
  • ES6 import – import * as Raven from 'raven-js'
  • TypeScript declare export – export = Raven
  1. Method two, ES6:
  • CommonJS export – exports.default = <object>
  • CommonJS import – var Raven = require('raven-js').default
  • ES6 import – import Raven from 'raven-js'
  • TypeScript declare export – export default Raven

This library uses method one and so the declared export is already correct.

The rest of the PR doesn't look offensive to me, but I don't know enough about Sentry to say whether it's all correct. Ideally all the new definitions should have tests to go along with them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants