Consider adding typescript definitions

[daniel-white]

Your package looks awesome. Would it be possible to add typescript definitions to your package and/or to definitelytyped? Thanks!

[davidparsson]

Hi!

That sounds like a good idea, but I don't currently have the time to provide that. Are there any tools for that may assist in that matter?

Pull requests would be appreciated, if applicable here. The API is rather simple and it is well defined by the tests.

[davidparsson]

I'm glad to let you know that I've now started working on this.

[cweatureapps]

That would be amazing! Our codebase is in TypeScript and this is what is holding us back from adopting this framework. Keep up the great work!

[dakoenig]

I just added type definitions via https://github.com/DefinitelyTyped/DefinitelyTyped/pull/64004. Those should be available via npm soon and can be used while the TS rewrite is in progress.

[davidparsson]

Thanks a million @dakoenig!

My ambition is still to convert this to TypeScript, but I got stuck on a few minor issues, and then my personal life got in the way.

[davidparsson]

I'm not sure if this helps any TypeScript users, but JSDoc type information was added in #61 and released today in 3.2.0.

[HarelM]

In general, the easiest solution would be to take the file from definitely typed, add it to this repo and update the package json to have it in the npm package and publish a new release. I can open a PR to do it if needed.

A more complex way would be to convert the code written here from javascript to typescript (it shouldn't take more than an hour I believe looking at the amount of code in this repo) and generate the typings at build time using rollup with typescript and dts or in any other way. I did it for a package that I maintain, it's not a lot of work: https://github.com/maplibre/maplibre-gl-inspect/pull/10

[davidparsson]

@HarelM, I started writing this:

I'd gladly accept a pull request with that. Are there any tools that can help making a best effort guess of whether the types match or not?

But I still have the ambition to convert this to TypeScript at some point, and hopefully sooner than later, although I'm not sure if it's tomorrow or in two years. I guess it boils down to how eager you are to get this. Any improvement is welcome!

[HarelM]

I can help with the conversion to typescript if this is the direction you are aiming for. Is rollup an option too to do the bundling and typescript definition generation from your perspective?

[davidparsson]

Awesome! That would be very much appreciated!

It has been a few years since i shifted my primary professional focus away from JavaScript/TypeScript, so I'm no longer keeping track of the best practices and which tools to use. That said, Rollup seems like a perfectly fine alternative for this.

It might also be worth mentioning that I've been wanting to transition away from using Grunt in favor of plain npm scripts, and possibly from Jasmine to Jest (or something else) for tests, so there is no need to keep any of them if they are in the way of this transition. Finally, I would like to keep the backwards compatibility if it's possible with a reasonable effort. If I recall correctly, I had some problems with that and the exported variable instance in src/index.js.

[HarelM]

I've created a PR with most of the above required changes (jest, typescript, etc). I didn't end up using rollup as I simply did a one-to-one typescript to javascript conversion, which seems to work as expected, I think, so I kept things as simple as I could. PR is here:

• #64

Let me know if there's anything I can help with.

I also added the ability to report coverage, so feel free to use that with codecov and any other service you'd like. I think the coverage is good, but I haven't dug into it, just made sure the tests pass basically.

[SimeonC]

@HarelM Thanks for your work converting fully to typescript!

@davidparsson It looks like the typescript types were merged in and a version notes added for v4.1.0 in the README but npm doesn't have version 4.0.0 or 4.1.0 available for download (should be noted that the package version is also still 3.2.1)

Could we get those published?

[davidparsson]

Oops, something must be wrong with my release script. Thanks for reporting!

I'll try to get the releases out there within a day or two, and drop a line here once it's done.

[davidparsson]

@SimeonC, 4.0.0 is released with all of these changes. It seems the only error was me forgetting to actually publish anything. Thanks for all the help and the reminder.

[HarelM]

@davidparsson please note that not all the types are exported from the index.d.ts file, see here: https://unpkg.com/browse/[email protected]/lib/index.d.ts Only the builder's type is exported, but not the return values of some of the methods, for example TestSuite. In my case I pass the test suite as a parameter to a method, and I need to know the internal structure of this library to do that, which is less than ideal: import type {TestSuite} from 'junit-report-builder/lib/test_suite'; I would be better to export the types from the index.ts file directly to keep the internal folder structure "hidden".

[HarelM]

Also there's a mismatch between the exported javascript code and the typescript definitions. In order to make version 4.0 work I need to do the following workaround:

import {default as BuilderExport, type Builder} from 'junit-report-builder';
const jnuitReportBuilder = (BuilderExport as any).default as Builder;

Let me know if you want me to open an issue/PR/something else to push this forward.

[davidparsson]

Thanks for reporting. Your point about having all the types exported from index.d.ts absolutely makes sense.

And I believe you, but I don't see why the types could differ. Do you?

I thought prepublish should be invoked and rebuild all types before publishing, but perhaps the release-it config breaks that?

And I also assumed that any typing errors would be caught by building/linting the end-to-end tests. Does not Jest help with that? Or do the tests simply lack typed variables?

Apparently, having three young kids and properly managing an open source library is hard, even if it's a small one. So yes, I appreciate any help I can get.

[davidparsson]

@HarelM, to be clear, I'd really appreciate a PR if you're up for it. No need to open a new issue.

[HarelM]

I'll take a look and let you know what I find.

[HarelM]

Adding the export for the types is easy, what I see is the following: Readme is saying to use the library like this:

var builder = require('junit-report-builder');

// Create a test suite
var suite = builder.testSuite().name('My suite');

index.ts file has this code:

export default new Factory().newBuilder();

e2e however has this code, which is passing the tests but is different from what is stated before and how this package was used in version 3.x:

import builderPackage, { type Builder } from '../lib';
...
beforeEach(() => (builder = builderPackage.newBuilder()));

The last line is different from how it used to be in the past, and how the readme is telling users to use this package.

I'm not sure if this was an intended change or not as part of version 4.0. Let me know how you want it to be and I'll change it accordingly.

[HarelM]

I think there's a problem with how typescript exports and code vs how it generate the d.ts file for commonjs. I tried a few things but none of them worked. Maybe there's a need to add rollup here to solve this, not sure what is the easiest solution...

[SimeonC]

Il try have a look this week sometime, I've dealt with this kind of problem countless times at work.

As a workaround for now you can use something like this: type TestCase = ReturnType<typeof builder["testCase"]> playground

[HarelM]

This is the bottom line in terms of diff between 3.x and 4.x, but the types do not reflect that for some reason: Note the exports.default vs module.exports

[davidparsson]

Is it possible to combine the two, having a variable exported by module.exports and exporting more internal types? It seems to me that it probably can't work without dirty tricks. But I might very well be wrong here.

[SimeonC]

Had a look around this morning, apart from lack of extra types I think the only issue is that the README is now wrong.

With the v3 -> v4 change and the typescript build the package changed from an CJS to an ESM, so import builder from 'junit-report-builder' is the correct way to import it now.

import {default as BuilderExport, type Builder} from 'junit-report-builder';
const jnuitReportBuilder = (BuilderExport as any).default as Builder;

@HarelM Could you tell me the value you have for package.json > type (one of be "module" "commonjs" or just not set) and if you're importing in typescript, or just in plain js files? Was trying to replicate the issue you mentioned and can't figure out the right combination of settings.

[HarelM]

@SimeonC here's the relevant commit from dependabot that I have updated in order to pass CI: https://github.com/maplibre/maplibre-gl-js/pull/4457 You can test is for yourself to see what is going on. Generally speaking, it uses rollup with commonjs plugin to build the code there, it uses type: module.

[SimeonC]

@HarelM Thanks, that was very helpful and I could manage to replicate the error you mentioned above.

I think this PR should fix that issue, or at least it does for me using npm link locally with the same setup as you linked. Unfortunately fixing the problem meant I had to fix a whole bunch of other small weird knock-on effects from re-configuring typescript 😅 - https://github.com/davidparsson/junit-report-builder/pull/71

[davidparsson]

I have published these changes in 4.0.1, and this time I made a pre-release and tested it properly. Big thanks again to @HarelM and @SimeonC!

When I find the time I will extend the test suite to make sure that both the CommonJS module and ES module is tested, and that the documentation examples work.