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

feat(): type definition for signale #25

Merged
merged 38 commits into from
Mar 15, 2019

Conversation

rjoydip-zz
Copy link
Contributor

@rjoydip-zz rjoydip-zz commented May 27, 2018

Type definition for Signal.

@jannislehmann
Copy link

+1
Hopefully this gets merged as I could use the typings currently.

index.d.ts Outdated
export interface Stream { }
export interface Config { displayScope?: Boolean; displayBadge?: Boolean; displayDate?: Boolean; displayFilename?: Boolean; displayLabel?: Boolean; displayTimestamp?: Boolean; underlineLabel?: Boolean; underlineMessage?: Boolean; underlinePrefix?: Boolean; underlineSuffix?: Boolean; uppercaseLabel?: Boolean; }
export interface Options { times?: Times; types: Types; stream?: Stream; scope?: String; }
export interface CompleteArrgumentType { prefix?: String; message: String; suffix?: String; }
Copy link
Contributor

Choose a reason for hiding this comment

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

You have misspelt argument. Unless you're a pirate ☠️

Copy link
Contributor Author

@rjoydip-zz rjoydip-zz Jun 2, 2018

Choose a reason for hiding this comment

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

@jordrake Can you tell me which one I misspelled? 😕

Choose a reason for hiding this comment

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

CompleteArrgumentType

Copy link
Contributor Author

@rjoydip-zz rjoydip-zz Jun 2, 2018

Choose a reason for hiding this comment

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

@Cludch and @jordrake Thanks for pointing me. Now, this is fixed.

@klaudiosinani klaudiosinani added the enhancement New feature or request label Jun 6, 2018
@klaudiosinani klaudiosinani mentioned this pull request Jun 9, 2018
@klaudiosinani klaudiosinani self-requested a review June 9, 2018 15:51
@resir014
Copy link

Hey @rjoydip @klauscfhq,

As mentioned on #39, I accidentally started work on my own version of the type declarations without checking here if someone had done it first. It's currently awaiting PR on DefinitelyTyped, but I can port them over here and I'd be glad to contribute to this PR if needed. :)

Thanks!

@rjoydip-zz
Copy link
Contributor Author

@resir014 Your type declarations is far better than my one. So if you give me the permission then I will take your code in here. Once it merged then anybody can extend. Otherwise, I will close this and you can make new PR.

@resir014
Copy link

@rjoydip Sure, no problem :) And feel free to add your name + github to the header as well.

@rjoydip-zz
Copy link
Contributor Author

@resir014 Thank you so much.

@rjoydip-zz
Copy link
Contributor Author

Awesome.

@resir014
Copy link

@rjoydip Alright, that's all that I could review for today, I need to go to bed.

I'll have another look tomorrow, but IMO this should be good to merge for now.

@rjoydip-zz
Copy link
Contributor Author

Yes. @resir014 Thanks once again. It couldn't be good without your help.

@rjoydip-zz
Copy link
Contributor Author

rjoydip-zz commented Jun 10, 2018

As described in issue #42. When i was checking signale.success(Symbol('test').toString()); I am getting this error error TS2349: Cannot invoke an expression whose type lacks a call signature. Type 'SymbolConstructor' has no compatible call signatures.

have to check this one

@resir014
Copy link

@rjoydip Alright. If you want me to, you can also give me push access to your fork and I can work on it as well. :)

@rjoydip-zz
Copy link
Contributor Author

@resir014 Please check it I gave you the access.

We no longer need them as we already base everything off of tsconfig
@resir014
Copy link

Thanks @rjoydip!

I'm going to make a few changes:

  • I've renamed the tests folder to the types folder, and moved the signale.d.ts file in there.
  • This means we can include the .d.ts file inside the types folder and not worry about the compiler errors on the Map interface.
  • Attempted to fix the commonjs default export issue

@rjoydip-zz
Copy link
Contributor Author

No problem. Change it

@resir014
Copy link

resir014 commented Jun 11, 2018 via email

};

export = signale;
export as namespace signale;

Choose a reason for hiding this comment

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

The export as namespace declaration is used with modules that export some sort of global variable. See here and here. Since Signale appears to be a CommonJS-only module and doesn't export a global, this line can probably be removed.

Choose a reason for hiding this comment

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

Fixed. 👌

@resir014
Copy link

@klauscfhq this should be ready to review 👌

@Eke
Copy link

Eke commented Jun 20, 2018

Hello, any updates on this? I would love to have this in my project.

@resir014
Copy link

@Eke Hi, I'm still pushing for this to be merged as well. I've updated the typings with the missing config objects from version 1.2.0 and now this should be ready for @klauscfhq to review and merge.

@klaudiosinani klaudiosinani requested review from klaudiosinani and removed request for klaudiosinani March 15, 2019 20:04
Copy link
Owner

@klaudiosinani klaudiosinani left a comment

Choose a reason for hiding this comment

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

Thank you all a lot for taking the time to contribute and work on this one!

@klaudiosinani klaudiosinani merged commit de6bb2e into klaudiosinani:master Mar 15, 2019
@rjoydip-zz rjoydip-zz deleted the ts-definition branch March 16, 2019 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants