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

Add typings for gaikan module #211

Merged
merged 3 commits into from
Mar 19, 2016
Merged

Conversation

SomaticIT
Copy link
Contributor

Typings URL: [https://github.com/typed-contrib/gaikan]
Source URL: [https://github.com/Deathspike/gaikan]

@blakeembrey
Copy link
Member

I think you'll need to remove browser from your typings, not sure why it's there. Is there a browser resolution version on the package?

@SomaticIT
Copy link
Contributor Author

The gaikan module is different depdending on the context.
In node, it contains more properties and more functions.
Maybe should I do like I did with knockout (#212 #213) ?

@blakeembrey
Copy link
Member

Probably, yes. Though the TypeScript UMD support would be better to hold out for. Unfortunately, browser does not do what you think it does. See https://github.com/typings/typings/blob/master/docs/faq.md#how-do-i-write-typings-definitions and https://github.com/defunctzombie/package-browser-field-spec.

Edit: UMD will be shipped with 2.0 - microsoft/TypeScript#7264.

@SomaticIT
Copy link
Contributor Author

@blakeembrey I think I understood correctly the browser entry in package.json.

As you can see in gaikan documentation, there is two different API depending on the context :
https://github.com/Deathspike/gaikan#7-server-api
https://github.com/Deathspike/gaikan#8-browser-api

Moreover if you look at the entry point file index.js, you can notice that the real entry point is different depending on the context :

module.exports = typeof window === 'undefined' ? 
        require('./server') : 
        require('./browser'); 

So I think my typings are correctly defined. I improved them during my tests.

@blakeembrey
Copy link
Member

Ah, you're right. This is kind of hacky, but since TypeScript doesn't support this kind of detail I'll merge 😄 You should prompt to author to instal use the browser field in package.json too - it'll result in smaller builds for their users.

blakeembrey added a commit that referenced this pull request Mar 19, 2016
Add typings for gaikan module
@blakeembrey blakeembrey merged commit 5e39ff4 into typings:master Mar 19, 2016
@SomaticIT SomaticIT deleted the gaikan branch March 20, 2016 22:36
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.

2 participants