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 type definitions #2

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Diluka
Copy link

@Diluka Diluka commented Oct 23, 2018

No description provided.

@dougwilson
Copy link
Contributor

Does the contents of the README really need to be replicated into the type definitions? Any way we can add a check to CI to prevent us from letting it get out of sync on accident if we have to do this?

@dougwilson dougwilson added the pr label Oct 23, 2018
@pillarjs pillarjs deleted a comment from coveralls Oct 23, 2018
@pillarjs pillarjs deleted a comment from coveralls Oct 23, 2018
@pillarjs pillarjs deleted a comment from coveralls Oct 23, 2018
@pillarjs pillarjs deleted a comment from coveralls Oct 23, 2018
@pillarjs pillarjs deleted a comment from coveralls Oct 23, 2018
@dougwilson dougwilson self-assigned this Oct 23, 2018
*/
declare function resolvePath(rootPath: string, relativePath: string): string;

declare namespace resolvePath {
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this do? Sorry not familiar with typings.

Copy link
Author

Choose a reason for hiding this comment

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

It's for typescript.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but I mean what exactly does an empty namespace so for typescript?

Copy link
Author

Choose a reason for hiding this comment

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

Removing the empty namespace will raise a type-check error in older version. It seems useless for current version. See the issue microsoft/TypeScript#5073

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, gotcha. I would have never known that, thank you. I would have accepted someone's PR removing it by accident. Can we add a test for this to prevent accidental removal?

Copy link
Author

Choose a reason for hiding this comment

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

How to test?

Copy link
Contributor

Choose a reason for hiding this comment

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

Idk, I'm not familiar with typescript. That's why I'm asking you :) if you're not sure, do you know someone who may be who van come and help out here?

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

Add a CI task to validate comments in typings are in sync with readme or remove readme copy from typings.

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

Successfully merging this pull request may close these issues.

2 participants