types & restructure

[daKmoR]

What I did:

Soooo this is a meatier change 😅

• For the types to make sense I removed the childrenPropertyName option => how important is that... could not get that typed... if important might try taking another stab at it
• I also removed the dynamically switching of arguments for all the walk functions
• Walk functions are now always node.all(callback, { strategy: 'foo' })
• Removed the context... with arrow functions, this is now hardly ever used
• Replaced a lot of .call() by directly calling to make the code easier to read
• As most of these changes are quite HUGELY breaking I am asking what you think about this? you still wanna go forwards taking this as the next major version? or is it too big of a change?
• there are still 2-3 type issues but I wanted to get your feedback first
• all tests are still green (I disabled all childrenPropertyName tests)
• all tests are using the types

[joaonuno]

At a glance, I agree with all the proposed changes except the one for removing childrenPropertyName. I think it's a useful config.

[daKmoR]

soooo that took a few more tries and the help of a typescript expert (thx @bennypowers) ... but we now have the childrenPropertyName back and TreeModel uses 2 generics (1 for the model and one for the childPropertyName)

some types are still not perfect hence there are some unknown casting + ts ignores in the code - but overall it works and the types are more detailed than on the current master

IMHO this should be good enough to prepare a beta of a new major 💪

what do you think? ok to merge and then prepare a beta?

[joaonuno]

Got to admit that I need to get more acquainted with Typescript before accepting this. I don't get the need to parameterize the type of the childPropertyName at the Node level. It seems to me that we just need to know that it's an optional string with a default value of 'children'.

I opened your branch on VSCode and it's full of red lines under the type definitions. Not sure why, might just be issues with my setup...

[daKmoR]

I opened your branch on VSCode and it's full of red lines under the type definitions.

you will need to setup to use the codespace typescript version (it's 4.5 beta) as 4.4. does not support default values in "templates/generics" via jsDoc (which is used 🙈)

You can do this by opening the View -> command palette (usually shift + command + p) and typing "Typescript select typescript version"

to run the typescript checking/building via the command line you can execute "npm run types"

I don't get the need to parameterize the type of the childPropertyName at the Node level.

A node needs to statically know which property contains the children.

consider a Node with the model data { name: 'Peter' children: ['Max'] }.
Typescript should throw an error if the type is Node<{ name: string, children: string[] }> as children are not "filled" with Node children.

If the type is Node<{ name: string, children: string[] }, 'deps'> then it should be ok as now children is part of the model and the node children are in "deps" which can be empty

Another angle => Typescript can not access the runtime value of this.config.childrenPropertyName that's why you need to "define" it via types as well)

PS: this was not "typed" before e.g. model and children is currently any which means that typescript will not give any errors if your model or children are "wrong"... there is an issue for it #88
PPS: I fully understand that this is a lot to digest... I frankly never had to write such complex typescript myself 🙈 and I spent a considerable amount of time on it... so only seeing the outcome and not the many interactions is probably a lot 🙈
PPPS: it's still "just" javascript with jsdoc... the typescript checking is totally optional and you could simply remove typescript and all the code would still work with green tests... (also there is no build step)

[joaonuno]

you will need to setup to use the codespace typescript version (it's 4.5 beta) as 4.4. does not support default values in "templates/generics" via jsDoc (which is used see_no_evil)

You can do this by opening the View -> command palette (usually shift + command + p) and typing "Typescript select typescript version"

It worked, thanks!

PPPS: it's still "just" javascript with jsdoc... the typescript checking is totally optional and you could simply remove typescript and all the code would still work with green tests... (also there is no build step)

While I get more joy writing javascript and appreciate not having a build step, I wonder if this is the way people write js libs nowadays...

what do you think? ok to merge and then prepare a beta?

Thanks for this effort, let's merge this and then think about what's missing for the release. The lint script doesn't seem to be working. Also need to understand what needs to be distributed so that types work properly for typescript developers.

[daKmoR]

sounds good - will prepare a PR for the release

=> for lining I apparently wrote the wrong plugin name 🙈

will fix in the other PR ok?