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 diff2html typings and tests #10681

Merged
merged 1 commit into from
Aug 30, 2016
Merged

Conversation

rtfpessoa
Copy link
Contributor

case 1. Add a new type definition.

  • checked compilation succeeds with --target es6 and --noImplicitAny options.
  • has correct naming convention
  • has a test file with the suffix of -tests.ts or -tests.tsx.

}

export interface Diff2Html {
getJsonFromDiff(input: string, configuration?: Options): Result;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you export these functions in the namespace directly:

export function getJsonFromDiff( ... )

Then export the namespace from:

export = Diff2Html

this way both the module based usage and the global usage work as expected.

Copy link
Contributor Author

@rtfpessoa rtfpessoa Aug 18, 2016

Choose a reason for hiding this comment

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

@paulvanbrenk The module is exported in a weird way. If you look at here https://github.com/rtfpessoa/diff2html/blob/master/src/diff2html.js#L104

Are you sure I should really remove that?

FYI I am completely new to Typescript.

Copy link
Member

Choose a reason for hiding this comment

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

The reason @paulvanbrenk and I are suggesting this is that if you don't do it that way, then your consumers won't be able to reference the types you're exporting (like Diff2Html.Options).

Copy link
Member

@DanielRosenwasser DanielRosenwasser Aug 27, 2016

Choose a reason for hiding this comment

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

Basically export = captures the namespace, value, and type meanings at any given location. You want to make some of those things you've defined accessible to others, so it'd be something like

namespace Diff2Html {
    export interface Options { /*...*/ }
    // ...
    export functon getPrettyHtml(input: any, configuration?: Options): string;
}

declare module "diff2html" {
    export = Diff2Html;
}

Now your consumers can actually write

import d2h = require("diff2html");

let opts: d2h.Options = {
    // ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DanielRosenwasser I do not think I understand you.

I know what you want. But it does not seem that what you are suggesting me to do will compile to valid JS code that will work with the module.

currently the code compiles to:

var Diff2Html = require("diff2html")
var d2h = Diff2Html.Diff2Html

and in d2h will the the public api of my library.

If in the typescript definition I remove that when the code compiles it will not run since if I do just:

var d2h = require("diff2html")

d2h will not have the methods. Do you understand my question? Or am I being somehow confusing?

Copy link
Contributor Author

@rtfpessoa rtfpessoa Aug 27, 2016

Choose a reason for hiding this comment

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

@DanielRosenwasser please confirm here: https://github.com/rtfpessoa/diff2html/blob/master/src/diff2html.js#L104
and here: https://github.com/rtfpessoa/diff2html#node-module

fyi, I have no problem in changing I just think it will not work as expected. But I am new to TypeScript.

@paulvanbrenk paulvanbrenk added the Revision needed This PR needs code changes before it can be merged. label Aug 18, 2016
@paulvanbrenk
Copy link
Contributor

@DanielRosenwasser can you take a look here?

@rtfpessoa
Copy link
Contributor Author

rtfpessoa commented Aug 27, 2016

@paulvanbrenk @DanielRosenwasser since I am new to Typescript, can you point me to a guide where I can very this?

By verify I mean run the diff2html-test.ts and check the library mapping is ok.

@rtfpessoa
Copy link
Contributor Author

rtfpessoa commented Aug 27, 2016

@paulvanbrenk @DanielRosenwasser fyi, I already compiled it with tsc and the output is what I would write in plain JS.

@vvakame vvakame merged commit ba79abd into DefinitelyTyped:master Aug 30, 2016
@vvakame
Copy link
Member

vvakame commented Aug 30, 2016

LGTM. @DanielRosenwasser sorry, I merged this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Revision needed This PR needs code changes before it can be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants