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.

as suggested here: https://typescript-eslint.io/rules/indent/

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.

we can now define at initilization what type of args we expect for every registered plugin

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 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 added 19 commits February 11, 2023 09:34

build: add ts config

0c73636

test: add eslint ts config

cabf787

chore: add mixme module declaration

7bb5f95

refactor: add types to utils

d58812e

chore: add mixme basic typing

f58e384

test: replace formatting rules with prettier config

6f03de7

as suggested here: https://typescript-eslint.io/rules/indent/

refactor: add types to error

e41576f

refactor: add types to index

583239e

refactor(error): make code property public

078863d

refactor: add NormalizedPlugin transitive type

bd5ec81

refactor: ensure plugin.require is defined

aa158d9

refactor: fix toposorting

7e80463

refactor: better typing (but still loose)

e617fa0

build: move types in dist folder

e5e5ba1

build: bump artifacts

fe67cba

fix: fix wrong assignment source

6fec880

refactor: replace "any" with "object" type in generics

bf30149

style: fix indentation

dad942b

fix(sample): added missing mandatory plugin name

83ddc5f

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

LucasDemea added 4 commits February 11, 2023 10:45

chore: housekeeping

a31ac01

build: bump artifacts

2ae74fb

refactor: remove call default parameters

4def56c

feat: parametrize the whole library

eaa2c4b

we can now define at initilization what type of args we expect for every registered plugin

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

test: migrate call_sync tests

c4278ba

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

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

LucasDemea Feb 6, 2023

wdavidw Feb 7, 2023

LucasDemea Feb 7, 2023 •

edited

wdavidw Feb 7, 2023

LucasDemea Feb 7, 2023 •

edited

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

LucasDemea commented Feb 12, 2023 •

edited

wdavidw commented Feb 13, 2023

LucasDemea commented Feb 13, 2023 •

edited

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

Feat/typescript #2

Are you sure you want to change the base?

Feat/typescript #2

Conversation

LucasDemea commented Feb 2, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucasDemea Feb 7, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucasDemea Feb 7, 2023 • edited

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

LucasDemea commented Feb 12, 2023 • edited

wdavidw commented Feb 13, 2023

LucasDemea commented Feb 13, 2023 • edited

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 Feb 2, 2023 •

edited

LucasDemea Feb 7, 2023 •

edited

LucasDemea Feb 7, 2023 •

edited

LucasDemea commented Feb 11, 2023 •

edited

LucasDemea commented Feb 12, 2023 •

edited

LucasDemea commented Feb 13, 2023 •

edited