fix: enable name import #252
Conversation
| isNetworkOrIdempotentRequestError, | ||
| isRetryableError, | ||
| isSafeRequestError | ||
| } from './index.js'; |
There was a problem hiding this comment.
I confirm that name import is working here (and that axiosRetry is able to import default).
| @@ -58,7 +61,7 @@ | |||
| "bugs": { | |||
| "url": "https://github.com/softonic/axios-retry/issues" | |||
| }, | |||
| "types": "lib/esm/index.d.ts", | |||
| "types": "index.d.ts", | |||
There was a problem hiding this comment.
This project is CommonJS, so index.d.ts should have been the default, not the type definition under esm, so I fixed it.
There was a problem hiding this comment.
1st of all, thank you for your commitment @yutak23 and @rchl for your feedback
This lib is actually written using ESM (es/index.mjs) and then built with babel for cjs and esm
I believe we followed this guide back in the day to implement such compatibility.
Looking at that, I still think is a good approach and may be worth follow the TS part and migrate the implementation to TS.
So my proposal would be to:
- revert feat: export of type definitions, depending on whether it is ES Module or CommonJS #250 and publish as 3.9.1 to be back to the previous situation (reopen Argument of type 'AxiosInstance' is not assignable to parameter of type 'AxiosStatic | AxiosInstance' #159) as @rchl is suggesting
- work on the migration to TS: I'm not sure when I will be able to do it, I'll try this weekend
What do you guys think? Should that solve both issues #159 and the new one?
There was a problem hiding this comment.
I agree that reverting is the best way forward for now.
Those changes just raise too many questions.
There was a problem hiding this comment.
Looking at that, I still think is a good approach and may be worth follow the TS part and migrate the implementation to TS.
Migrate to TS seems easiest in the end. I agree that moving to TS would solve the problem
(I think we can handle both by setting up tsconfig for each of CJS and ESM as in https://github.com/stripe/stripe-node).
And it was not a good idea to make major changes without testing against TypeScript type definitions. I apologies.
@mindhells
I apologize for the inconvenience, but please close this PR at the appropriate time if necessary.
@rchl
Thank you for the pointers, etc. And I am sorry that my fault resulted in releasing something with bugs.
| export function isNetworkError(error: Error): boolean; | ||
| export function isRetryableError(error: Error): boolean; | ||
| export function isSafeRequestError(error: Error): boolean; | ||
| export function isIdempotentRequestError(error: Error): boolean; | ||
| export function isNetworkOrIdempotentRequestError(error: Error): boolean; | ||
| export function exponentialDelay(retryNumber?: number, error?: Error, delayFactor?: number): number; |
There was a problem hiding this comment.
Those were not exported directly before. Those were exported through AxiosRetry which should be exported instead.
I'd still want you to answer #250 (comment) more specifically because I'm not sure why the export = syntax doesn't work for you.
There was a problem hiding this comment.
Those were not exported directly before. Those were exported through AxiosRetry which should be exported instead.
This is to conform to the specification in JavaScript. It is also possible to name import the isNetworkError function as follows.
This time, it has been modified accordingly.
There was a problem hiding this comment.
Well, you can export those since it's a "new feature" but you also have to export AxiosRetry since it was exported before and it was possible to do import { AxiosRetry } from 'axios-retry' while it's not possible with those changes.
|
Ok guys, thank you both for your dedication and feedback on this. |
This ought to fix #251.
One possible cause of the degradation is the lack of type definition testing.
Therefore, we will propose the addition of type definition testing in this PR. This test is similar to testing JavaScrip code, so I do not think this will significantly increase maintenance costs.
Rather, it would greatly improve the reliability of TypeScript type definitions.
※The scripts are daringly separated in test:tsd because tsd does not work due to CI's Node.js version. Local test results are below (Node.js v18.17.0).
For Node.js v16.20.2
(I have not tested anything under Node 14 as I believe it has already reached EOL. cf. https://github.com/nodejs/Release#nodejs-release-working-group)