Feat/typescript #2

LucasDemea · 2023-02-02T16:51:44Z

This PR adds typescript support.

Closes #1

LucasDemea · 2023-02-06T17:25:00Z

lib/index.ts

+          return hook !== undefined;
+        }) as [Hook, Hook][][];
+      const edges = [...edges_after, ...edges_before].flat(0);
+      return toposort.array(mergedHooks, edges).map((hook) => {


@wdavidw maybe you could help with this part ? I can't get the edges type right.

I totally suck in Typescript, I had a look at it but no idea.

I understand that it can be intimidating if your not used to the syntax, but what I'm asking is actually quite "simple" and doesn't require to fully understand typescript. You can approach it as any pseudo type language (like the one you wrote in the readme for the parameters types).

You can also pull the PR locally to get some hints from your editor and eslint.

In fact I'm not sure if there is an issue with the typings, if I understood your code correctly, or if there is a bug in the original code.

Could you at least check lines L338 and L342 and tell me if as [Hook, Hook][]; and as [Hook, Hook][][]; are supposed to be the correct return types of the map+filter sequences ?

TS is complaining of receiving in the edges parameter (L369) a [Hook, Hook][][] type while it expects a [Hook, Hook | undefined][] one. What makes me think of an implementation issue is the additionnal [] array dimension that we have here.

I did pull the changes and I confirm that I saw the TS error both in the CLI and in VSCode complaining about the [Hook, Hook][][] and [Hook, Hook | undefined][]. However, I didn't know how to read those 2 last hints. I can give it another try

The as keyword in as [Hook, Hook][] for example, is a type cast, to indicate to TS what the hook.before.map(...).filter(...) return type is supposed to be. Here I use it after the filter because typescript is not able to infer that the resulting array doesn't contain any undefined values anymore. If you hover the filter keyword, you'll see what is the actual type TS is inferring ((Hook[] | undefined)[] in this case).

On L367 we end up with a [Hook, Hook][][], which is correct in regards to the current implementation (please confirm), but we need a [Hook, Hook][], as toposort.array expects. We should have one nesting level less.

I found a few things, tests are not passing but I got it to compile

error.ts, line 2: remove private, error code is public

index.ts, line 368: depth 0 in array_flatten convert to depth 1 in flat:
const edges = [...edges_after, ...edges_before].flat(1);

index.ts, line 369: pass the Hook trait and return hook from map
return toposort.array<Hook>(mergedHooks, edges).map((hook) => { if (hook) { if ('require' in hook) delete hook.require; if ('plugin' in hook) delete hook.plugin; } return hook });

Thank you, I will have a look

index.ts, line 369: pass the Hook trait and return hook from map
return toposort.array<Hook>(mergedHooks, edges).map((hook) => { if (hook) { if ('require' in hook) delete hook.require; if ('plugin' in hook) delete hook.plugin; } return hook });

This is not required, typescript infers the Hook type from arguments.

LucasDemea · 2023-02-10T20:34:09Z

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 · 2023-02-11T09:36:47Z

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 · 2023-02-12T20:46:42Z

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 · 2023-02-13T08:29:12Z

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 · 2023-02-13T10:19:51Z

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 · 2023-02-22T21:04:03Z

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

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 · 2023-02-23T09:13:56Z

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

LucasDemea · 2023-04-24T15:44:11Z

Hi @wdavidw, did you see my latest comments ?

wdavidw · 2023-04-25T07:44:13Z

@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 ?

LucasDemea · 2023-02-23T09:25:21Z

lib/index.ts

+export type HookHandler<T extends object> = (
+  args: T,
+  handler?: HookHandler<T>
+) => null | void | HookHandler<T> | Promise<HookHandler<T>>;


We should first agree on the HookHandler type because it has implications on many other parts. Does it look good to you ? Or should we broaden the return type to something more permissive ?

args is currently of type T extends object. But in your call_sync test, you pass an array. Maybe we should also broaden its type ?

@wdavidw Let me know if you need some explanations about the typings

@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 ?

My bad, my review was left pending ... 🤦

Hi @LucasDemea. args could be anything. The return value of the handler may also be anything. Honestly, TS always get me confused.

dec0dOS · 2023-09-07T15:09:40Z

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 · 2023-09-07T15:13:37Z

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

LucasDemea · 2024-07-02T05:57:36Z

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 · 2024-07-02T07:40:05Z

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 · 2024-07-02T11:22:08Z

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 · 2024-07-02T11:42:51Z

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 · 2024-07-02T12:05:05Z

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 · 2024-07-02T21:04:47Z

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 · 2024-07-03T02:07:58Z

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 · 2024-08-02T09:28:14Z

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 · 2024-08-25T16:57:10Z

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

LucasDemea force-pushed the feat/typescript branch 2 times, most recently from d2e1073 to 2af4b40 Compare February 6, 2023 17:20

LucasDemea commented Feb 6, 2023

View reviewed changes

LucasDemea force-pushed the feat/typescript branch from 2f32308 to 83ddc5f Compare February 11, 2023 09:28

LucasDemea marked this pull request as ready for review February 12, 2023 20:50

wdavidw force-pushed the feat/typescript branch from 41cdf63 to c4278ba Compare February 24, 2023 11:17

LucasDemea commented Apr 25, 2023

View reviewed changes

LucasDemea requested a review from wdavidw July 2, 2024 05:25

LucasDemea force-pushed the feat/typescript branch from 660a602 to e45bb0c Compare July 2, 2024 05:47

LucasDemea force-pushed the feat/typescript branch from 09b9d04 to 5355c43 Compare July 3, 2024 01:55

LucasDemea force-pushed the feat/typescript branch from 65f5cf5 to 4f0b0f3 Compare August 19, 2024 03:09

feat: ts conversion

5941a4d

wdavidw force-pushed the feat/typescript branch from 3c2bce8 to 5941a4d Compare August 25, 2024 16:37

wdavidw closed this in 6a8aae7 Aug 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/typescript #2

Feat/typescript #2

LucasDemea commented Feb 2, 2023 •

edited

Loading

LucasDemea Feb 6, 2023

wdavidw Feb 7, 2023

LucasDemea Feb 7, 2023 •

edited

Loading

wdavidw Feb 7, 2023

LucasDemea Feb 7, 2023 •

edited

Loading

LucasDemea Feb 7, 2023

wdavidw Feb 7, 2023

LucasDemea Feb 7, 2023

LucasDemea Feb 12, 2023

LucasDemea commented Feb 10, 2023

LucasDemea commented Feb 11, 2023 •

edited

Loading

LucasDemea commented Feb 12, 2023 •

edited

Loading

wdavidw commented Feb 13, 2023

LucasDemea commented Feb 13, 2023 •

edited

Loading

wdavidw commented Feb 22, 2023

LucasDemea commented Feb 23, 2023

LucasDemea commented Apr 24, 2023

wdavidw commented Apr 25, 2023

LucasDemea Feb 23, 2023

LucasDemea Feb 23, 2023

LucasDemea Mar 2, 2023

LucasDemea Apr 25, 2023

wdavidw May 9, 2023

dec0dOS commented Sep 7, 2023

wdavidw commented Sep 7, 2023

LucasDemea commented Jul 2, 2024

wdavidw commented Jul 2, 2024

LucasDemea commented Jul 2, 2024

wdavidw commented Jul 2, 2024

LucasDemea commented Jul 2, 2024

wdavidw commented Jul 2, 2024

LucasDemea commented Jul 3, 2024

wdavidw commented Aug 2, 2024

wdavidw commented Aug 25, 2024

Feat/typescript #2

Feat/typescript #2

Conversation

LucasDemea commented Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucasDemea Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucasDemea Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucasDemea commented Feb 10, 2023

LucasDemea commented Feb 11, 2023 • edited Loading

LucasDemea commented Feb 12, 2023 • edited Loading

wdavidw commented Feb 13, 2023

LucasDemea commented Feb 13, 2023 • edited Loading

wdavidw commented Feb 22, 2023

LucasDemea commented Feb 23, 2023

LucasDemea commented Apr 24, 2023

wdavidw commented Apr 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dec0dOS commented Sep 7, 2023

wdavidw commented Sep 7, 2023

LucasDemea commented Jul 2, 2024

wdavidw commented Jul 2, 2024

LucasDemea commented Jul 2, 2024

wdavidw commented Jul 2, 2024

LucasDemea commented Jul 2, 2024

wdavidw commented Jul 2, 2024

LucasDemea commented Jul 3, 2024

wdavidw commented Aug 2, 2024

wdavidw commented Aug 25, 2024

LucasDemea commented Feb 2, 2023 •

edited

Loading

LucasDemea Feb 7, 2023 •

edited

Loading

LucasDemea Feb 7, 2023 •

edited

Loading

LucasDemea commented Feb 11, 2023 •

edited

Loading

LucasDemea commented Feb 12, 2023 •

edited

Loading

LucasDemea commented Feb 13, 2023 •

edited

Loading