Feat/typescript

[LucasDemea]

This PR adds typescript support.

Closes #1

[LucasDemea]

David, you should be able to fix and adapt the tests now. There is still some work at the typing level (some any types that we must get rid of) but it compiles and should be testable.

[LucasDemea]

I'd like to hear your opinion about 2 things:

• I updated the linting & formatting configs. I replaced the outdated eslint config with a prettierrc, trying to replicate your original settings. I found out that some files weren't indented correctly. I indented them to match eslint & prettier rules. It added semicolons, single-quotes... Is it OK like this ?
• I'd suggest to remove the build artifacts from the repo as it is boring to update them with each commit. Instead we could add some github actions to automate the build & deploy to npm with each new release. I can take care of this in a separate PR if you want.

[LucasDemea]

There are 2 things I'd like to bring your attention to, in the last (and supposedly final) changes.

• I removed args and hooks default = [] (4def56c) as it doesn't fit with the declared types and I couldn't find any use for it. Please check there are no implications I could have missed.
• Last but not least : library authors can now type their library, forcing typescript users to give a specific args type to the handler function. Defaults to the object type. This is made possible via the use of typescript generics.

I couldn't run the tests but I imported the types in a library I'm developing. Everything works well so far.

[wdavidw]

It added semicolons, single-quotes... Is it OK like this ?

That's fine. My preference is no semicolons for personal projects, not settled for open source projects (csv packages are with semicolons), and single-quotes everywhere (coffeescript heritage).

I'd suggest to remove the build artifacts from the repo

I am hesitant about this, if a CI does it automatically, new extra commits will pollute the commit history.

library authors can now type their library,

I am not too familiar with ts generic but it seems like the way to go for dynamic/user types.

Could you give me write access to your fork, this way I could add the tests directly in this working branch. I hope to have some time end of the week.

[LucasDemea]

I'd suggest to remove the build artifacts from the repo

I am hesitant about this, if a CI does it automatically, new extra commits will pollute the commit history.

There wouldn't be more commit pollution than there currently is. My suggestion is to totally remove the dist folder, and let CI build & deploy to npm with each new release/tag, without adding anything to the git repo. The only commit you'd have in history is the same you already use for releases d2cd22f. I personnally also use https://github.com/googleapis/release-please for automating changelog updates based on conventional commits.

Could you give me write access to your fork, this way I could add the tests directly in this working branch. I hope to have some time end of the week.

You should already have write access.

[wdavidw]

Hi @LucasDemea, could you please do a review of my commit 41cdf637cfc36c477a109722ac9848d0b9da2a05

I am not too happy of my TS skills, here are a few notes:

• ./lib/index.ts line 92: update call_sync: (args: callArgs<T>) => any from unknown to obtain the value returned by call_sync in the tests
• ./lib/index.ts line 496: add // @ts-ignore because I couldn't find a way to pass a function, which from a type angle may be null but which from a runtime angle will never be null.
• tsconfig.json: "noImplicitAny" set to false because I could import the mixme.d.ts type definition otherwise

[LucasDemea]

I will have a look. Not sure I'll find the time this week though

[LucasDemea]

Hi @wdavidw, did you see my latest comments ?

[wdavidw]

@LucasDemea I don't see any new comment. To me, the last comment is "I will have a look. Not sure I'll find the time this week though". Am I missing something ?

[dec0dOS]

This looks promising! Seems to be before merging the PR I could publish the dist/index.d.ts to https://github.com/DefinitelyTyped/DefinitelyTyped Are there any objections to this?

[wdavidw]

Please propose a definition file. There is still a lot of work pending to complete this PR and no time to do it.

[LucasDemea]

Hi David, I finally found some time to work on the PR again.

what I did:

• completed the TS definitions
• added some build and linting tools
• added documentation
• modernized parts of the code to comply with eslint recommendations
• added typedoc generator

There are still some linting issues left, but I wasn't sure if I could fix them without modify your initial code logic. I'd prefer if you could have a look at these.

Tests need to be migrated to TS because coffeescript can't read typescript from what I understand. I got some DataCloneError issue while trying to run call_sync.ts tests. As I'm not familiar with mocha, maybe you could help with this ?

[wdavidw]

Hi Lucas, I have been thinking a lot about your PR in the last month. My main concern is how will I be able to manage the code base if I am barely able to write a unit test with it. It all come down to the decision to go full TS for which I am fully liable. I know you invested a lot of effort in the PR but would you agree to work on the definition file, instead of migrating lib/index.js to Typescript, and convert the tests to TS ?

[LucasDemea]

I see 3 possibilities here :

• I maintain a typescript version of the project in a fork
• I maintain d.ts files for the current JS project
• You try to learn typescript and we finish migrating the tests.

The tests could be written in almost pure JS if we disable TS compiler type checking features. These features could be enabled later. That could help with migrating progressively to TS.

Of course the most beneficial option overall would be the third, especially now that the hard work is done, but I assume you don't have the will or time to learn typescript, and I respect that.

On the other hand, I don't see myself throwing away all these code improvements and go back to a far from perfect solution (the second one). Actually this is what I was doing the past week, before jumping back to migrating the code base to full typescript : by integrating your library in one of my project and using the definition file extracted from the first version of my fork, some types issues appeared. I had to fix them directly in the definition file. This was a lot more complicated and imprecise than writing full TS code, and that brought me to the idea of completing the full rewrite once and for all, which is obviously more optimal.

I'm left with the first option. I would have to migrate the tests from coffee script to typescript. That shouldn't be to difficult as I saw some existing tools to do this automatically.

What are your thoughts about this ?

If you decide to try to understand the TS code I could take a couple of hours to explain the code to you via a call and liveshare (I saw you're based in Paris, so you must be speaking french. I'm french too).

[wdavidw]

I am proposing a combination of number 2 and 3. We could start collaborating in writing the tests in TS against a definition file on your branch. Once all the tests pass, I will be happy to give you contributor privileges on the repo. From there, we could discuss whether a full TS implementation make sense. You are correct, I am based in Paris and speak french.

[LucasDemea]

Ok, I will try to fix the call_sync.ts test file now, so that you will understand how tests can work with typescript. Typescript is a javascript superset, this implies we can write the tests in pure javascript and typescript will be able to read them. We only have to disable TS typechecking for this. This is the purpose of // @ts-nocheck in the first line in call_sync.ts.

[wdavidw]

Back in the days, I did managed to have at least one or two test passing in call_sync.ts. However, if I remember correctly, the code in call_sync was switched with the one from call. Did you notice this ?

[LucasDemea]

call_sync.ts is now working. 4/5 tests pass, when typechecking is disabled. Could you please check ? Just update your dependencies and run yarn test. I left an indication in the file, if you want to enable type checking, but then only the first test will compile. The other tests won't, because of type issues.

[wdavidw]

Hi Lucas, I spent the last few days trying on my side to implement plug-and-play motivated by me need to put my hands into TS. There are a few thing I am happy about but so many things for wish I am at best not confident. Could you have a look at the feat/typescript-david branch.

[wdavidw]

@LucasDemea your commit are squashed and merged into master in commit 6a8aae707caf7bb9210da7bf8e3c62f5ce359889