Adding Typescript definitions

[bendtherules]

Hi @joaonuno,
Would you like to include/maintain Typescript definition files for this module inside the repo?

I have created the type definition file, which I hope is reasonably complete right now. Currently, it is a open PR to the DefinitelyTyped repository (which is where you should add the type definitions in case it is not possible to accomodate it in this original repo).

1. It would be great if we could maintain it inside this repo directly so that lib users can directly benefit from those type hints just by installing this module from npm.
2. If not, can you please look through the changes made in the PR, if you have some time.

Either of this will be very beneficial for the purpose.

[joaonuno]

Maintainers of DefinitelyTyped have more experience reviewing these PRs. If the PR ends up in the DefinitelyTyped repo, would it still make sense to include it here?

Can we add some tests to tree-model using type script that use these type definitions to ensure no regressions are introduced with new commits? Is this a good approach to ensure types are correct?

[bendtherules]

It should stay in either one of these places. Its generally recommended that if possible, it is maintained within the repo itself, rather than DefinitelyTyped because

1. Reviewers generally (I think) don't actually check that the type definitions represent the actual js lib code. They check that the type definitions itself are correct.
2. You are not allowed to write js tests that import the actual npm module. Only typescript tests have to be written which import the type definition and are checked for linting and type checking issues only.
3. It is not exactly linked to the npm lib version. The compatible version of the lib has to be mentioned, but when someone installs @types/libName, they will always get the latest type definition irrespective of the libName version that they are currently using.
4. Users will have to separately do npm install @types/libName to get the type definitions. If we keep it in the repo, they get it directly when installing the lib.

On the other hand, the benefit of keeping it there is that they have strict meta-rules setup for contributions, which ensures that lint and type errors are always checked. Also, the benefit of review from typeScript experts.

Can we add some tests to tree-model using type script that use these type definitions to ensure no regressions are introduced with new commits?

Sounds like a good approach. Although I must admit, I am also new to TypeScript land and not exactly aware of project management standards. To accomplish this though, we will need to keep it in the repo - because of problem 3 (not linked to lib version).

[joaonuno]

So, if I understood correctly, 2. 3. and 4. are solved if the type definitions are added to this lib repo directly and some simple typescript tests are added to use the types, right?

Regarding your last concern, can we add those strict meta-rules to the npm lint script in this repo?

[joaonuno]

I'm not familiar with typescript so I don't know how easy it would be to add some tests here...

[bendtherules]

Your understanding is correct.

Yes, we can add lint and test rules similar to what they have added. And also probably, take cues from other popular repos.

I think it won't be hard to add the typescript tests here. I can spend some time on this.

[joaonuno]

Okay, if you do spend time on this and get to a good result, send your PR and I'll review it.

Thanks!

[bendtherules]

Great.

I would like to understand two more things about the project:

1. Can we use some pattern other than let TreeModel = require("tree-model"), something that will allow us to do something like let {TreeModel, Node} = require("tree-model")?
   This will help with types because with the existing pattern, as I understand, the user won't be able to import the type definition of Node. More explanation
2. Is there any requirement to commit the dist folder? Doesn't look like a good practise.

[joaonuno]

1. I'll leave that investigation up to you, I have nothing against exporting the Node type.

2. Yes, check issue #40 for more details.

[bendtherules]

Thanks, got all the answers.

[bendtherules]

Finally, have something ready for this. Sending it as PR.
All the magic was in this project - https://github.com/Microsoft/dtslint. DefinitelyTyped itself uses this for those meta checks.

Its sort of a linter + test-runner combo - checks the type definition file and checks that the type test file is correctly typed according to the typedef file. It also allows you to assert about the expected types and expected error by calling with wrong signatures (in the type test file).

From the previous comment https://github.com/Microsoft/dtslint#add-types-for-a-library-not-on-definitelytyped, this still doesn't solve point 2. Someone has to manually check that the types/tree-model-tests.ts file uses valid tree-model api.

^ There seems to be two ways to solve this -

1. Re-write tests in TypeScript with mocha-typescript
2. Write duplicate tests for TypeScript ??

Even without either of these, the current solution seems reasonably acceptable to me right now. Most projects (which are not heavy on TS) also don't bother to add TS tests,

[joaonuno]

Thank you for this effort, at a glance seems very solid! I'll pick this during this week (or next weekend).

[joaonuno]

I've started looking at your PR and trying the code locally. I had never tried Typescript and It's really cool to use the lib with types. I have a few questions for you to help me understand this.

• Reading here, they say:

Use module-class.d.ts if your module can be constructed using new

This is the case with TreeModel. Do you know why is this recommended?

• Can you explain me why we have the TreeModel class and the TreeModelMisc namespace?

[bendtherules]

Hey @joaonuno ,
I looked into the file again and yes, we probably should keep the class and namespace name same.

My only concern originally was that it would look like we are again referencing TreeModel within the class TreeModel (though actually we are referencing to the interface here):

declare class TreeModel {
    constructor(config?: TreeModel.Config);

    private config: TreeModel.Config;

    parse(model: TreeModel.Model): TreeModel.Node;
}

But if we later want to export the namespace and we keep its current name (TreeModelMisc), we would need some way to export it using alias TreeModel - but currently, aliasing namespace during export (something like: export TreeModelMisc as namespace TreeModel) is not implemented.

[bendtherules]

Small bump.

Any updates on the review?

[pie6k]

Seems like ts definiton is missiong some fields (eg node.children, node.parent)

[bendtherules]

Seems like ts definiton is missiong some fields (eg node.children, node.parent)

You are right. We need to add those to node type.
Please add a PR if you would like to fix this.