Update typings for Typescript 2.0 #190

AaronAsAChimp · 2016-11-17T20:06:33Z

Description

Typescript 2.0 has a new mechanism for resolving types for modules. This PR updates the typings for react-datetime to be compatible with with the latest typescript. It also includes a couple of fixes and enhancements to the typings.

Restrict viewMode to the correct strings (or a number).
Improve typings for timeConstraints
Fix renderDay, renderMonth, renderYear to the correct return types.
Improve typings for inputProps, limiting it to only those allowed on input elements
Fix ReactDatetime to be a class

Motivation and Context

Fixes issue #181. Improves compatibility with this package.

Checklist

My change required changes to the documentation.
- I have updated the documentation accordingly.
- I have updated the TypeScript type definition accordingly. :P
I have added tests to cover my changes.
All new and existing tests passed.

simeg · 2016-11-17T23:02:28Z

typings/index.d.ts

@@ -46,15 +67,11 @@ declare module ReactDatetime {
     */
    locale?: string;
    /*
-     Whether to interpret input times as UTC or the user's local timezone.
-     */
-    utc?: boolean;


Why was this prop removed?

... a mistake.

simeg · 2016-11-17T23:04:10Z

typings/index.d.ts


-//// <reference path="../moment/moment-node.d.ts" />
+declare module "react-datetime" {


Here you are using double-quotes but on L17 you are using single-quotes. Does the module declaration require double-quotes? If not I think you should change them to stay consistent :)

simeg · 2016-11-17T23:05:42Z

typings/index.d.ts

+
+  /*
+   A stand-in type for Moment, this file currently has no way of guaranteeing
+   the existence of those typings.


I like comments like these 😌

simeg · 2016-11-17T23:09:34Z

typings/react-datetime-tests.tsx

@@ -0,0 +1,170 @@
+import * as React from 'react';


Is this the most appropriate place for this file? If I was looking for a test file in a repo I'd check in the tests folder (where our other tests are).

I was following the recommendations from the DefinitelyTyped repository. This one doesn't necessarily apply here, I can move it.

So in the DefinitelyTyped readme it says

File: foo-tests.ts
Purpose: This contains sample code which tests the typings. This code does not run, but it is type-checked.

And that's what's happening in your changes, right? I didn't get that at first but yeah if strictly the typings are being tested I can see them living in the folder they're in now.

I see the typings being a separate modular thing on top of the component, so if this is the case (please confirm) then I'd say just leave it as is.

Yes, that is definitely the case 😄

simeg · 2016-11-17T23:15:32Z

typings/react-datetime-tests.tsx

@@ -0,0 +1,170 @@
+import * as React from 'react';
+import { Moment } from 'moment';


This file is written with ES6 syntax. I prefer ES6 syntax, but none of the other files are written in this syntax. My first thought is that we should stay consistent, but at the same time this could be the first step to making everything into ES6 (which I plan on doing eventually). What are your thoughts on this?

I prefer using ES6 syntax, the code is cleaner and allows for better static analysis. It is also the preferred style for Typescript.

All right, let's go with ES6.

simeg · 2016-11-17T23:19:19Z

typings/react-datetime-tests.tsx

+		renderMonth={ (props, month, year, selectedDate) => {
+			return <td {...props}>{ month }</td>;
+		} }
+		renderYear={ ( props, year, selectedDate ) => {


You have spaces in the arguments here that you don't have on L118 or L121 (sorry if this is a bit picky).

simeg · 2016-11-17T23:20:15Z

typings/tsconfig.json

+    },
+    "files": [
+        "index.d.ts",
+        "react-datetime-tests.tsx"


If moving the test file this reference will have to be updated as well (just a friendly reminder).

simeg · 2016-11-17T23:43:07Z

Hey @AaronAsAChimp, thanks for this awesome contribution! As you can see I added some comments to the code.

These are breaking changes for people using version < 2.0 right? Is there some way to make it compatible for both, or has the whole community already embraced version 2?

AaronAsAChimp · 2016-11-18T20:40:07Z

You're correct they are breaking changes. DefinitelyTyped maintains a separate branch for types-2.0 to preserve the 1.8 typings. I wasn't able to find a way to make these typings work for both.

simeg · 2016-11-18T21:19:36Z

I am trying to figure out if we can support both v1.8 and v2.0, but I can't really see a way where we don't have to maintain two branches/repos, something I'd prefer not having to do.

I am leaning towards merging this, because I like the tests and when looking at the breaking changes from 1.8 -> 2.0 there's not really a lot of them. @AaronAsAChimp what are your thoughts? Any idea on how we can support both versions?

vutran · 2016-11-18T23:30:45Z

+1 for the TS 2.0 support. If people are still on TS 1.8, they most like are not ahead in updating their packages as well (in this case, react-datetime).

simeg · 2016-11-18T23:36:57Z

@vutran that is true, but usually dependencies are marked with a carrot (^) which means it wont update to the next major version but minor and patch versions are ok. This PR would be a minor version bump for react-datetime but the TS version bump 1.8 => 2.0 is a major.

Maybe I'm overthinking this, I just don't want to answer to people asking why we broke their application. I welcome every opinon on this matter :)

schickling · 2016-11-20T09:49:03Z

Would be great if @AaronAsAChimp could provide this via @types until this is merged. As in fact it's breaking our build since we're using Typescript 2.1.

AaronAsAChimp · 2016-11-22T18:42:13Z

I would prefer we either:

Commit to having the typings here for the latest typescript and move the 1.8 typings and any future legacy typings to DefinitelyTyped, or
move all typings there and leave this repository to be JavaScript only.

Having them in both places would only lead to confusion, extra maintenance, and erroneous bug reports when someone inadvertently uses stale typings. Even if its only temporary. But Its not really up to me, its up to the maintainers and how much time they are willing to spend supporting these typings.

simeg · 2016-11-22T21:03:48Z

@AaronAsAChimp I like the first option you are suggesting. In a few months we could look into only using v2.0+ in both DefinitelyTyped and here.

@AaronAsAChimp @vutran @schickling I am reading the instructions in the README for the DefinitelyTyped repo, and I'm getting the feeling that some of you people that actually use TS can publish the 1.8 version quicker and better. Does anyone feel up for it?

In what order do we do this? Can we merge this now and publish the 1.8 version to DefinitelyTyped later?

Thanks so much for the help and input everyone!

bovan · 2016-12-05T12:16:11Z

typings/react-datetime-tests.tsx

@@ -0,0 +1,171 @@
+import * as React from 'react';
+import { Moment } from 'moment';
+import ReactDatetime = require('react-datetime');


this should be equal to
import * as ReactDatetime from 'react-datetime';

but it's not.. i get the following error:
[ts] Module ''react-datetime'' resolves to a non-module entity and cannot be imported using this construct.

I now get no errors in vscode using import ReactDatetime from 'react-datetime'; , but its not able to import it properly so it breaks during build

is it possible to make it work using the import * as name syntax?

@AaronAsAChimp would you be able to answer this? Thanks so much.

Sorry it took so long to reply, but I wanted to make sure I had it right. They are not equivalent.

The following is for CommonJS modules that use module.exports = Thing. This may be an ugly way of importing, but it is not within the scope of this PR to change it. It would also massively break the way non-typescript users require the module.

import ReactDatetime = require('react-datetime');

When you have multiple named exports module.exports = { Thing: Thing } or export class Thing { ... } it is imported like so.

import * as ReactDatetime from 'react-datetime';

Finally, when you have a default export: export default class Thing {...}

import ReactDatetime from 'react-datetime';

You can read more here
https://www.typescriptlang.org/docs/handbook/modules.html#export--and-import--require

@AaronAsAChimp That's what I suspected. Thanks for your input, and thanks a lot for creating the PR in DefinitelyTyped.

@bovan does this make sense?

@AaronAsAChimp @simeg I suspect the documentation isn't up to date.. the require syntax is what typescript implemented before the ES6 syntax was ready.

The main issues here are:

Editors does not allow you to type import * as ReactDatetime from 'react-datetime'; which works

Editors allows you now to type: import ReactDatetime from 'react-datetime'; which doesnt work.

This indicates that there is something wrong with the typings. (Also all other imports we use have no problems with the import * as- syntax, and there's a lot).
I'm sure there is a way to make the typings work without rewriting the actual code itself, unfortunately I've not had time to do further research into this.

Anyway, the problem for us is that we use tslint with "no-var-requires": true, but i work around that with disable-line, so it's not game breaking for me, just a heads up..

simeg · 2016-12-07T17:19:04Z

About this PR - I want to wait to see what happens to the PR in DefinitelyTyped. If it gets merged I will merge this but if not I will have to think about the next step.

Add tests for typings. Fixes issue arqex#181

Remove useless settings from tsconfig. (by recommendation of the DefinitelyTyped maintainers)

AaronAsAChimp · 2016-12-12T19:41:10Z

With the DefinitelyTyped pull request closed. Is this ready for merge?

I've updated the branch with the suggested changes.

simeg · 2016-12-20T19:44:46Z

Sorry for the late response everyone. So we did end up with supporting both 1.8 and 2.0+ in the same repo. I am ok with that if that's the suggestion from the DefinitelyTyped people, so I'll go ahead and merge this PR.

Thanks a lot everyone who contributed and a big thank you to @AaronAsAChimp for everything.

simeg reviewed Nov 17, 2016

View reviewed changes

AaronAsAChimp mentioned this pull request Nov 30, 2016

Add typings for react-datetime for typescript 1.8 DefinitelyTyped/DefinitelyTyped#13022

Closed

11 tasks

bovan reviewed Dec 5, 2016

View reviewed changes

bovan mentioned this pull request Dec 7, 2016

Fixed es6 style imports AaronAsAChimp/react-datetime#1

Closed

AaronAsAChimp added 7 commits December 7, 2016 15:55

Update typings for Typescript 2.0

bde26a0

Add tests for typings. Fixes issue arqex#181

Add Typescript information to Readme.

df91f6b

Clean up readme changes.

78589f4

Code review fixes.

8d2e702

Add 1.8 typings back

d85a166

Remove useless settings from tsconfig. (by recommendation of the DefinitelyTyped maintainers)

Add a hack to make imports cleaner.

24d6940

Add the 1.8 typings to the NPM install.

dbaa927

AaronAsAChimp force-pushed the 181-typings-fix branch from b6bdf28 to dbaa927 Compare December 7, 2016 21:01

simeg merged commit a20f583 into arqex:master Dec 20, 2016

simeg mentioned this pull request Dec 20, 2016

Typings not compatible with Typescript 2.0 #181

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update typings for Typescript 2.0 #190

Update typings for Typescript 2.0 #190

AaronAsAChimp commented Nov 17, 2016 •

edited

Loading

simeg Nov 17, 2016

AaronAsAChimp Nov 18, 2016

simeg Nov 17, 2016

simeg Nov 17, 2016 •

edited

Loading

simeg Nov 17, 2016

AaronAsAChimp Nov 18, 2016

simeg Nov 18, 2016

AaronAsAChimp Nov 18, 2016

simeg Nov 17, 2016

AaronAsAChimp Nov 18, 2016

simeg Nov 18, 2016

simeg Nov 17, 2016

simeg Nov 17, 2016

simeg commented Nov 17, 2016

AaronAsAChimp commented Nov 18, 2016

simeg commented Nov 18, 2016 •

edited

Loading

vutran commented Nov 18, 2016

simeg commented Nov 18, 2016

schickling commented Nov 20, 2016 •

edited

Loading

AaronAsAChimp commented Nov 22, 2016

simeg commented Nov 22, 2016

bovan Dec 5, 2016

simeg Dec 7, 2016

AaronAsAChimp Dec 7, 2016

simeg Dec 7, 2016

bovan Dec 7, 2016

simeg commented Dec 7, 2016

AaronAsAChimp commented Dec 12, 2016

simeg commented Dec 20, 2016


		//// <reference path="../moment/moment-node.d.ts" />
		declare module "react-datetime" {

		@@ -0,0 +1,170 @@
		import * as React from 'react';
		import { Moment } from 'moment';

Update typings for Typescript 2.0 #190

Update typings for Typescript 2.0 #190

Conversation

AaronAsAChimp commented Nov 17, 2016 • edited Loading

Description

Motivation and Context

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simeg Nov 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simeg commented Nov 17, 2016

AaronAsAChimp commented Nov 18, 2016

simeg commented Nov 18, 2016 • edited Loading

vutran commented Nov 18, 2016

simeg commented Nov 18, 2016

schickling commented Nov 20, 2016 • edited Loading

AaronAsAChimp commented Nov 22, 2016

simeg commented Nov 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simeg commented Dec 7, 2016

AaronAsAChimp commented Dec 12, 2016

simeg commented Dec 20, 2016

AaronAsAChimp commented Nov 17, 2016 •

edited

Loading

simeg Nov 17, 2016 •

edited

Loading

simeg commented Nov 18, 2016 •

edited

Loading

schickling commented Nov 20, 2016 •

edited

Loading