feat: migrate to typescript

[mindhells]

Migrate to Typescript

Following this guide: https://www.sensedeep.com/blog/posts/2021/how-to-create-single-source-npm-module.html

[mindhells]

This relates to #250, #251 and #252

Please @yutak23 @rchl have a look

[rchl]

I will have a more proper look from the interoperability standpoint once other comments are clarified/addressed.

[rchl]

This might be something from before but I wonder what's the reasoning for adding those package.json files in the first place? I'm pretty sure that those don't really have any effect since it's the package's main package.json that decides whether something is an esm or commonjs module.

[mindhells]

Awesome feedback @rchl
Thanks a ton!
I believe I have addressed everything. Had to do some extra adjustments in the internal typings for everything to gear up.
Feel free to resolve the conversations as you think they are correctly addressed.
Looking forward for your feedback from the interoperability stand point 🙇‍♂️

[mindhells]

@rchl about removing @eslint/recommended, I had to, because otherwise it complains about "unused params"... in the interface declarations 🤷

[rchl]

Feel free to resolve the conversations as you think they are correctly addressed.

Please resolve them. I, not being a contributor or maintainer, can't resolve them. I find it weird about github but oh well...

[rchl]

@rchl about removing @eslint/recommended, I had to, because otherwise it complains about "unused params"... in the interface declarations 🤷

So typescript-eslint documentation suggests this: https://typescript-eslint.io/linting/configs#eslint-recommended

(Keep eslint rules and only extend with @typescript-eslint/eslint-recommended. That should disable eslint rules that are not compatible with typescript.)

[rchl]

@rchl about removing @eslint/recommended, I had to, because otherwise it complains about "unused params"... in the interface declarations 🤷

So typescript-eslint documentation suggests this: typescript-eslint.io/linting/configs#eslint-recommended

(Keep eslint rules and only extend with @typescript-eslint/eslint-recommended. That should disable eslint rules that are not compatible with typescript.)

I do see the issue also when testing it myself though. Though I think it's better to manually disable no-unused-vars than removing eslint:recommended because otherwise we'd loose a lot of checks that are not present in the typescript rules.

[rchl]

It looks good to me. Just run npm i again because package lock is not fully up-to-date.

[mindhells]

@rchl My plan is to publish this as a minor, as there shouldn't be any breaking changes in the public API. WDYT?

[rchl]

I would not vouch with my life that it won't break anything but I agree that it shouldn't.

There is added peer dependency which is sort of a breaking change typically but technically it should have had that since the beginning so it seems more like a bug fix.

I suppose it's also good to wait for @yutak23 to check it out first.

[yutak23]

@mindhells

This setup would result in the following error.

$ npx tsc
srv/app.ts:5:1 - error TS2349: This expression is not callable.
  Type 'typeof import("/home/study/workspace/ts-node-oidc/node_modules/axios-retry/dist/index")' has no call signatures.

5 axiosRetry(axios);
  ~~~~~~~~~~

A draft amendment is prepared below. We hope you will review it.

#255

[yutak23]

@mindhells

If you are moving to typescript, I don't think you need to bother with namespace. Also, the presence of this namespace would be a destructive change.

Specifically, this code does not export IAxiosRetryConfig, etc.

I would appreciate it if you could refer to the PR of the proposed fix below for more details.

#256

[yutak23]

If we want to keep namespace, we would need to export each one separately as follows. However, this is redundant, so I think a modification that eliminates the namespace declaration is fine.

export type AxiosRetry= IAxiosRetry.AxiosRetry;
export type IAxiosRetryConfig = IAxiosRetry.IAxiosRetryConfig;
export type IAxiosRetryConfigExtended = IAxiosRetry.IAxiosRetryConfigExtended;
export type IAxiosRetryReturn = IAxiosRetry.IAxiosRetryReturn;

[mindhells]

Thanks a ton @yutak23, amazing job, I have incorporated both of your fixes 🙇

[mindhells]

After @yutak23 fixes I understand we are safe to publish this a minor, are we?

[rchl]

I find this creation of package.json in subdirectories weird and hacky. I have never seen it being done elsewhere.

Since the exports object defines whether something is esm or cjs, I don't get why it would be necessary to also add this package.json.

I saw this approach mentioned in the guide linked in this PR but it's not really explained much there and I haven't seen it used anywhere else.

Do you know of any specific case that would fail without it?

[yutak23]

While certainly a bit hackneyed, this is one effective method. This method is https://nodejs.org/api/packages.html#dual-package-hazard. The following is a summary of how the Dual Package can be achieved and is informative.

microsoft/TypeScript#49462 (comment)

And this method is also taken by other libraries such as stripe-node.
https://github.com/stripe/stripe-node/blob/v14.5.0/package.json#L64

Do you know of any specific case that would fail without it?

If we do not do this, even if you separate them into directories esm, cjs, the module system is still determined by the module settings in package.json's type(in this situation type is not set, so assumed to be a cjs setting since there is no type setting).
Therefore, you will get the following compile error.

This expression is not callable.
  Type 'typeof import("/home/study/workspace/ts-node-oidc/node_modules/axios-retry/dist/esm/index")' has no call signatures.ts(2349)

This error occurs when deleting package.json from esm in node_module.

This approach is necessary if you want to support both esm and cjs from a single TypeScript file, but it is a different story if you use .cts or .mts extensions like axios.

[rchl]

OK. I trust what you are saying.
But just gonna note that the official Node documentation doesn't mention those extra package.json's so it's weird to me that that would be needed.

I wanted to also verify it on my own but I could not reproduce it on my side with this setup and extra package.json's removed:

But I can see that Typescript insists on importing from /cjs/ so that's probably why but I'm not sure why it's doing that...

[mindhells]

@rchl @yutak23 Thanks a lot for your contributions to this PR, you are awesome!
I'm publishing this as v4, even though no breaking changes should have been introduced (a part from what @rchl commented on the peerDeps) I think there's no reason to take the risk and create a new minor.