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

typescript typings #1521

Merged
merged 1 commit into from
Jan 13, 2020
Merged

typescript typings #1521

merged 1 commit into from
Jan 13, 2020

Conversation

ondratra
Copy link
Contributor

@ondratra ondratra commented Aug 6, 2019

Summary of proposed changes

Hi, I have created and tested Typescript types for the project that were missing.

Checklist

  • Use develop as the base branch
  • Exclude the gulp build (/dist changes) from the PR
  • Test on supported browsers

@ondratra
Copy link
Contributor Author

@sampotts can you check this PR, please?

@mats852
Copy link

mats852 commented Sep 27, 2019

In the meanwhile, if anybody needs the type definitions in their project without rebuilding the package themselves, simply add this file to your src:
https://gist.github.com/mats852/5e3a749e0ac050e2d01bb735b7b27cce

I've wrapped @ondratra 's work in a module definition :)

Copy link
Owner

@sampotts sampotts left a comment

Choose a reason for hiding this comment

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

Thanks for doing this 👍

@sampotts sampotts merged commit 3424d08 into sampotts:develop Jan 13, 2020
@smnbbrv
Copy link
Contributor

smnbbrv commented Mar 11, 2020

@ondratra is there any reason to use export = Plyr; instead of export default Plyr;?

The current implementation is very confusing since the plyr documentation uses default export all over again and export = Plyr; forces to import * as Plyr from 'plyr' instead of import Plyr from 'plyr'..

@smnbbrv
Copy link
Contributor

smnbbrv commented Mar 11, 2020

@sampotts could you confirm that default export is a "proper" way to go?

@ondratra
Copy link
Contributor Author

ondratra commented Mar 11, 2020

@smnbbrv Some clarity on the topic of export = Something vs export default Something can be found here microsoft/TypeScript#7185 .

The way I import Plyr into my project is not via default import - default doesn't seem to work, that's why the typings reflects that via using export = Plyr.

import * as Plyr from 'plyr'
import Plyr2 from 'plyr'

console.log(Plyr) // ƒ t(n,i) {...}
console.log(Plyr2) // undefined

I guess it might seem confusing because src/js/plyr.js file uses ES import/export, but generated final package (located in dist/plyr.js) that you will actually import to your project uses CommonJS - and typings correspond to this final package.

@smnbbrv
Copy link
Contributor

smnbbrv commented Mar 12, 2020

@ondratra hm. It works in my project if I use default export... Could you remember what was the issue?

My problem is that I received a PR smnbbrv/ngx-plyr#35 which is targeting this and now I'm slightly confused. No worries, I can use * as Plyr thing; the question is that it's a shared library and I would love to follow the official documented way to use Plyr to avoid furhter typings etc problems.

That's why I'd love to see @sampotts opinion in this conversation.

@ondratra
Copy link
Contributor Author

@smnbbrv The issue is mentioned in my last post - Plyr2 variable is undefined. Check out this sandbox and compare it's tsconfig.json to yours, maybe you have some extra options that make it work for you. https://repl.it/@ondratra/plyrTypingsTest If you remove allowSyntheticDefaultImports: true from tsconfig.json in sandbox, it doesn't compile over that default import.

Strangely enough, at the time of writing that sandbox imports empty object from plyr and I don't know a reason why. =-O But it should return a function like my previous post suggested.

@smnbbrv
Copy link
Contributor

smnbbrv commented Mar 14, 2020

@ondratra thanks for claryfying, and great thanks for your work!

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