add typescript definition

[pofider]

Thank you for your work. There are some things I would like to discuss about it.

Your single definition file now strictly define types, however jsreport is very much dynamic and final types are composition of core and installed extensions. The jsreport-phantom-pdf for example extends the template type with phantom property where the header or footer can be defined. It would be great if we can use declaration files merging or other typescript concept to reach the same in typescript types. Every extension would then provide its definition file which would be merged in. The developer would then see proper types based on the extensions imported. We would also don't need to maintain one big files covering everything.

What you think about it? Would it be doable?

[bjrmatos]

hi! thanks for the PR. here is some things that come to my mind with this addition

• one thing to consider is the maintenance cost of this addition, i'm not a huge fan of having type definitions in a codebase which does not use types, feels weird and it will be more work for us to keep the definitions updated.

• i think that type definitions for repositories that does not use typescript at all should live in definitelytyped and should be maintained by typescript users which knows best how to express API in such language. we are not typescript users (in jsreport) so it feels weird to me if we are going to start maintaining types for something that we don't use (of course that can change in the future but just describing the present)

please don't see my comments as a rejection 😄, i'm just trying to describe the current situation..

thanks for your work, i'm sure that this will be a great start for having jsreport typed (whether if the types live in our own repository or in definitelytyped).

[pofider]

The discussion here kind of froze, but I would really like to continue with this because the start proposed in PR is good. The code will never be merged here and will in the end live in definitelytyped, but that is not the reason to stop.

I think the main problem to resolve now is the definition files merging to allow extensions to participate on the final type as I described in this comment.

[taoqf]

Sorry for seeing your replies so late. I do not know much about jsreport, Is it something jsreport kind like jquery? I've made a pr in DefinitelyTyped.

[pofider]

The jquery plugins extends the core $ type. jsreport extensions extends the jsreport core types and enums.

Your PR defines for example the template type like this:

template: {
        content: string;
        engine: 'jsrender' | 'handlebars' | 'ejs' | 'jade';
        helpers: string;
        recipe: 'phantom-pdf' | 'electron-pdf' | 'text' | 'xlsx' | 'html-to-xlsx' | 'phantom-image' | 'html-to-text' | 'fop-pdf' | 'client-html' | 'wrapped-html' | 'wkhtmltopdf';
    };

However this is not full type when using even single extension jsreport-phantom-pdf which extends the template with property phantom: { header: string ..... }. It would be great if every extension can add its own props to the core types using some kind of types merging. I believe it could be doable as this is common problem also with mentioned jquery.

Regarding the PR to DefinitelyTyped. I don't know what is the majority opinion here. I would rather close it and bake the final form here and as we are ready send the final form there. But maybe I'm wrong. In every case the current form needs some refinements and usage of merged types instead of naming everything in one place.

@taoqf Would you have time to play with the types merging? I've tried it quickly without success, but it should work somehow.

[taoqf]

Yes, I'd like to fix the types, infact

recipe: 'phantom-pdf' | 'electron-pdf' | 'text' | 'xlsx' | 'html-to-xlsx' | 'phantom-image' | 'html-to-text' | 'fop-pdf' | 'client-html' | 'wrapped-html' | 'wkhtmltopdf' | string;

may be an easier way to define the types.

[taoqf]

@pofider , I can even give some test code here:

import * as inline from 'inline-source';
import * as JsReportCore from 'jsreport-core';

const jsreport = JsReportCore();

export default function html2pdf(html: string) {
    return new Promise<Buffer>((resolve, reject) => {
        inline(html, {
            compress: true,
            // rootpath: path.resolve('www'),
            // Skip all css types and png formats
            ignore: ['png']
        }, (err, content) => {
            jsreport.init().then(() => {
                return jsreport.render({
                    template: {
                        content,
                        engine: 'jsrender',
                        recipe: 'phantom-pdf'
                    }
                }).then((resp) => {
                    // fs.writeFileSync('./a.pdf', resp.content);
                    resolve(resp.content);
                }, (e) => {
                    reject(e);
                });
            }).catch((e) => {
                reject(e);
            });
        });
    });
}

[pofider]

The ... | string makes it better. I see.

However now you want to do

return jsreport.render({
   template: {
     content,
     engine: 'jsrender',
     recipe: 'phantom-pdf',
     phantom: { header: 'Hello world' }
  }
})

It would be great if we can merge types from phantom into the root template type.

[taoqf]

interface RenderOptions {
    template: {
        content: string;
        engine: 'jsrender' | 'handlebars' | 'ejs' | 'jade';
        helpers?: string | { [fun: string]: helpers };
        recipe: 'phantom-pdf' | 'electron-pdf' | 'text' | 'xlsx' | 'html-to-xlsx' | 'phantom-image' | 'html-to-text' | 'fop-pdf' | 'client-html' | 'wrapped-html' | 'wkhtmltopdf';
        phantom?: any;
    };
    data?: any;
}

or

interface RenderOptions {
    template: {
        content: string;
        engine: 'jsrender' | 'handlebars' | 'ejs' | 'jade';
        helpers?: string | { [fun: string]: helpers };
        recipe: 'phantom-pdf' | 'electron-pdf' | 'text' | 'xlsx' | 'html-to-xlsx' | 'phantom-image' | 'html-to-text' | 'fop-pdf' | 'client-html' | 'wrapped-html' | 'wkhtmltopdf';
        [key: string]: any;
    };
    data?: any;
}

I really do not know what you want to do.

[taoqf]

Are you guys would merge this pr? I have closed https://github.com/DefinitelyTyped/DefinitelyTyped/pull/22913 already. if not, I will reopen that pr.

[pofider]

I will try to explain on example again what I would like to reach. Please read my previous comments again to get the context.

To shortly repeat again. The problem here is that you create one giant file with definitions from all extensions (> 50). The downside of this is that everybody who creates and publish an extension needs to send a PR to single jsreport types. The second problem with one giant definition file is that user who only use 3 extensions gets types from all of 50 which is very misleading. It would be much better if the types are split into dedicated packages @types/jsreport-core, @types/jsreport-scripts, @types/jsreport-images....

Example

// core.d.ts
interface Reporter {
    coreProp: string
}

declare function init (): Reporter;
export = init

// extensionA.d.ts
interface Reporter {
    // adding additional property to the core Reporter interface
    extensionAProp: string
}

declare function extensionA(Reporter): Reporter;
export = extensionA

// main.ts
import core = require("jsreport-core")
import extensionA = require("extensionA")

const reporter = core()
extensionA(reporter)

I would expect that the definition files core.d.ts and extensionA.d.ts are merged in this case and the final Reporter interface includes both props coreProp and extensionAProp. The problem is that this doesn't work for me. The types merging works only if I put everything into the single file. Do you know if this could work somehow? The documentation for declaration-merging doesn't say anything how this could work if multiple declaration files are used. Thank you.

[taoqf]

Should this work? @pofider

[pofider]

Wonderful. Excellent work! Thank you. This works for me. This is what I wanted.

This is so powerful that the jsreport-core can even specify the enum type for engine as

declare namespace JsReport {
enum Engine {
    none = 1
}

interface Template {
    content?: string;
    engine?: Engine | string;       
}

interface RenderOptions {
    template: Template,
    data?: any;
}
}

and the jsreport-jsrender extension can even add its own value to the enum as

declare namespace JsReport {
    enum Engine {
        jsrender
    }
}

And the intellisense works nice afterwards

I was a bit afraid of this, but as this works everything else should as well.

To further proceed, we will need to cut out some of the types from the main jsreport core and move them to the respective extensions. I think I will send a minor PR to the @types/jsreport-core and then we can create the types for the popular extensions.

[taoqf]

I would rather we could maintain type definitions in this project.

[pofider]

I've finished the changes in definition files and sent you PR for review if you find time, please look at it. Thank you.

I would rather we could maintain type definitions in this project.

This would be easier for us. However the common practice is different. The projects that doesn't use typescript mostly store types externally and it seems to me reasonable.

[taoqf]

I have tried my best to take a preview.

This would be easier for us. However the common practice is different. The projects that doesn't use typescript mostly store types externally and it seems to me reasonable.

maybe these projects who doesn't care about typescript should put them into DefinitelyTyped. or we could contribute, if the authors would like to merge the prs

[taoqf]

pls see https://github.com/jsreport/jsreport-jsrender/pull/2 https://github.com/jsreport/jsreport-phantom-pdf/pull/25 https://github.com/jsreport/jsreport-html-to-xlsx/pull/14 and https://github.com/jsreport/jsreport-xlsx/pull/31 thank you. @pofider

[pofider]

I think this version could be final for now. What you think? I would like to take the changes here and send prs to DefinetelyTyped. Create a dedicated folder there for each extension. As this:

DefinitelyTyped/DefinitelyTyped/types/jsreport-core DefinitelyTyped/DefinitelyTyped/types/jsreport-jsrender DefinitelyTyped/DefinitelyTyped/types/jsreport-phantom-pdf DefinitelyTyped/DefinitelyTyped/types/jsreport-html-to-xlsx DefinitelyTyped/DefinitelyTyped/types/jsreport-xlsx

Would you be able to do this?

[taoqf]

Now we have choice:

1. put all these types into one single file and maintain them in this project, at the same time, other people could add extends by themselves, like we did before(maybe we need a guide in readme).
2. put all these types into one single file and maintain them in DefinitelyTyped
3. put these types into separate files and maintain them in their own projects.
4. put these types into separate files and maintain them in DefinitelyTyped

I prefer 1, because I think it may stop contributors who want write a type definition or just send an issue when they have problems with the type definition in DefinitelyTyped, it's very strict in DefinitelyTyped.

I think DefinitelyTyped's purpose is to help people who want use a project in npm (or not in) without type definition in typescript, so if a project start support typescript in their own project, it should be removed in DefinitelyTyped.

Anyway ,choose one, I will do the work with pleasure.

[taoqf]

@pofider Have you seen this?

[taoqf]

I will be away for couple of days, I have commit a pr in DefinitelyTyped https://github.com/DefinitelyTyped/DefinitelyTyped/pull/23523

[pofider]

Thank you very much. I apologize for the delay.

I did check many the top ranked packages in npm and the majority of them hosts the types in DefinitelyTyped. I understand it can be more difficult to maintain it there, but I would like to try it. If it will be pain for us, we can always remove it there and start to host types directly in our projects.

put these types into separate files and maintain them in DefinitelyTyped

This approach with splitting types into folders for each particular extension sounds better. Because developers then get only types they need as they import extensions. The developer using 2 extensions gets exactly types he needs. Not all of them.

[taoqf]

I am back!!! https://github.com/DefinitelyTyped/DefinitelyTyped/pull/23848

[pofider]

Excellent! Thank you. The DT PR looks good.

[taoqf]

unfortunately, we could not export const enum.

[pofider]

Hm, I was not careful with checking the changes in DT. Why we could not export const enums? This is was the great part of it, so developer can dot and list recipes and engines.

[taoqf]

Because these packages do not export that enume actually, I mean in js, and exporting const enum is not the suggested way in DT.