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 moment-timezone types definition #3577

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

Add moment-timezone types definition #3577

wants to merge 3 commits into from

Conversation

GrapFinanceDev
Copy link

Found out we don't have moment-timezone's type definition.

Copy link
Member

@pascalduez pascalduez left a comment

Choose a reason for hiding this comment

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

You'll need to write tests.
Check the contributing guidelines

@pascalduez pascalduez added the changes requested Changes have been requested by a reviewer label Sep 28, 2019
@GrapFinanceDev
Copy link
Author

@pascalduez will add some test cases later.

@GrapFinanceDev GrapFinanceDev changed the title Add moment-timezone types definition [WIP]Add moment-timezone types definition Oct 8, 2019
@GrapFinanceDev GrapFinanceDev changed the title [WIP]Add moment-timezone types definition Add moment-timezone types definition Oct 11, 2019
@GrapFinanceDev
Copy link
Author

@pascalduez update import problem now.

* Flowgen v1.10.0
*/

import * as moment from "moment-timezone";
Copy link
Member

Choose a reason for hiding this comment

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

That's quite confusing.

@pascalduez
Copy link
Member

Still missing tests.

@Brianzchen
Copy link
Member

If this definition is to some day get merged, the definition shouldn't be built on flow-gen I believe. It's just way too confusing.

From what I understand moment-timezone is same as moment plus a few more features with tz().

Also the definition should target package version 0.5.x not 2.x.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes requested Changes have been requested by a reviewer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants