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 support #106

Closed
riley-stroud-ck opened this issue Feb 24, 2017 · 18 comments
Closed

Typescript support #106

riley-stroud-ck opened this issue Feb 24, 2017 · 18 comments

Comments

@riley-stroud-ck
Copy link
Contributor

Are you planning on adding support for typescript?

@hustcc
Copy link
Owner

hustcc commented Feb 24, 2017

Can you do something to help me by a pr?

@likerRr
Copy link
Collaborator

likerRr commented Feb 24, 2017

hope, #107 can help

@hustcc
Copy link
Owner

hustcc commented Feb 24, 2017

666

@likerRr
Copy link
Collaborator

likerRr commented Feb 25, 2017

@hustcc lol, what's that?

@hustcc
Copy link
Owner

hustcc commented Feb 26, 2017

6 has the same pronunciation with 牛 in Chinese, 牛 is cow, but has meaning of perfect, or very good in somewhere.

666(three 6) means very very very good, and admire you for your develop ability.

@likerRr
Copy link
Collaborator

likerRr commented Feb 27, 2017

@hustcc wow, very instructive, thank you

@rileystroud can you please try this branch and confirm if it works for you? You can install it through npm like:

npm install hustcc/timeago.js#ts-support

@hustcc
Copy link
Owner

hustcc commented Feb 27, 2017

@rileystroud merge into master, so just npm install hustcc/timeago.js for test.

@likerRr
Copy link
Collaborator

likerRr commented Feb 27, 2017

@hustcc probably you need to update npm version for that and update dist files (sorry, I didn't make it in #110, because I didn't know you could merge it so quickly)

@riley-stroud-ck
Copy link
Contributor Author

It's not working for me on my first pass. I'll get back to it in a couple hours

@likerRr
Copy link
Collaborator

likerRr commented Feb 27, 2017

@rileystroud how did you install it? can you check if timeago.d.ts present in your node_modules/timeago.js folder?

@riley-stroud-ck
Copy link
Contributor Author

riley-stroud-ck commented Feb 28, 2017

It installed fine. The typescript code wasn't compiling to the correct javascript.

import timeago from 'timeago.js'
const timeagoInstance = timeago()
timeagoInstance.format('01-01-2017')

compiled to

const timeago_js_1 = require("timeago.js");
const timeagoInstance = timeago_js_1.default();
timeagoInstance.format('01-01-2017');

but there is no default function defined.

With #112,

import * as timeago from 'timeago.js'
const timeagoInstance = timeago()
timeagoInstance.format('01-01-2017')

compiles to the expected

const timeago = require("timeago.js");
const timeagoInstance = timeago();
timeagoInstance.format('01-01-2017');

@likerRr
Copy link
Collaborator

likerRr commented Feb 28, 2017

I checked out your version (#112), it doesn't even compile with typescript ^2.2.1:

app.ts(1,8): error TS1192: Module '"timeago-test/node_modules/timeago.js/timeago"' has no default export.
Command failed: tsc

Which version of typescript do you use?

@riley-stroud-ck
Copy link
Contributor Author

riley-stroud-ck commented Feb 28, 2017

I'm on typescript 2.2.1
@likerRr Did you update the import statement as above?

@likerRr
Copy link
Collaborator

likerRr commented Feb 28, 2017

Yes, sorry, you was right, I incorrectly imported the module. Finally it works for me.

But, honestly, I have doubts. Your variant works fine, but syntax import * as timeago from 'timeago.js' is quite redundant. It's readed as "import all the members as timeago namespace\variable", but in fact there is only one exported member - timeago factory. I think, that interfaces TimeAgoConstructor and TimeAgo shouldn't be part of the timeago factory, but as separate entities. Also such typings add some trash (interfaces) to autocomplite in IDE.

Second moment - let's look on the problem from other side. If we write clear typescript file:

// hello.ts
export default function hello() {};

and compile it with such ts config:

{
  "compilerOptions": {
    "module": "commonjs",
    "target": "es5"
  }
}

Generated js file will be:

"use strict";
exports.__esModule = true;
var Hello = (function () {
    function Hello() {
    }
    return Hello;
}());
exports["default"] = Hello;

As far as we see, default becomes a member of exports, and this file becomes ES6-import style compatible. The same story if we try to compile such code with babel:

// hello.js
export default function hello() {};

// hello.transpiled.js
"use strict";

Object.defineProperty(exports, "__esModule", {
  value: true
});
exports.default = hello;
function hello() {};

Both Typescript and Babel (webpack 2 does the same, but output is quite bigger) transpile default ES6 style import to ES5 style by adding default property to exports. So, if we want timeago to become compatible with ES6 default import (I mean support syntax like import timeago from 'timeago.js'), we need modify a module pattern a little:

// now
  if (typeof module === 'object' && module.exports)
    module.exports = factory(root);

// will become
  if (typeof module === 'object' && module.exports) {
    module.exports = factory(root); // nodejs support
    module.exports['default'] = module.exports; // es6 support
  }

@hustcc It will also solve this issue #105

If we do so, then typings that are currently in the project will become working.

So, we should decide:

  1. Should we support ES6 default import compatible with commonjs?
    1.1. If yes, then leave typings as they are for now and modify module pattern implementation. Issue ES6 #105 will be solved itself
    1.2. If no, accept PR by @rileystroud that fixes typings, no other modification is needed. Issue ES6 #105 needs more research.

Probably I misunderstand something, correct me if I'm wrong

@hustcc
Copy link
Owner

hustcc commented Mar 1, 2017

@rileystroud @likerRr I think export default is the reason. So I think method 1.1 is better, also can solve #105.

@hustcc hustcc closed this as completed Mar 1, 2017
@estk
Copy link

estk commented Mar 31, 2017

Shouldn't the constructor return an instance of itself?
(Notice the this in the new() signatures.)

/// <reference types="jquery" />
type TDate = Date | string | number;
export interface TimeAgoConstructor {
    new (): this;
    new (nowDate: TDate, defaultLocale?: string): this;
    format(date: TDate, locale?: string): string;
    render<T>(nodes: Node | NodeList | JQuery, locale?: string): void;
    setLocale(locale: string): void;
}
export interface TimeagoFactory {
    (): TimeAgoConstructor;
    (nowDate: TDate, defaultLocale?: string): TimeAgoConstructor;
    cancel(node?: Node | JQuery): void;
    register(locale: string, localeFunc: Function): void;
}
declare let timeagoFactory: TimeagoFactory;
export default timeagoFactory;

@likerRr
Copy link
Collaborator

likerRr commented Mar 31, 2017

I hope this keyword is implied. Anyway now it doesn't generate compilation errors

@likerRr
Copy link
Collaborator

likerRr commented Apr 3, 2017

@estk constructor is removed since the timeagoFactory returns an instance #126

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

No branches or pull requests

4 participants