-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace axios with (node-)fetch #358
Conversation
@@ -29,7 +27,7 @@ type Options = Xor< | |||
* The URL of the root of the Mollie API. Default: `'https://api.mollie.com:443/v2/'`. | |||
*/ | |||
apiEndpoint?: string; | |||
} & Pick<AxiosRequestConfig, 'adapter' | 'proxy' | 'socketPath' | 'timeout'>; | |||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should document that this is a breaking change.
Fixes #346. |
What's missing from these changes, is an explicit networkMocker.cleanup().
Tests now call networkMocker.cleanup() ‒ which in turn calls nock.restore() ‒ to ensure tests don't affect each other.
In 3.7.0, DELETE requests without body sometimes cause "{}" to be sent, and sometimes causes no body to be sent at all. Demian changed this, causing no body to be sent consistently. This commit causes "{}" to be sent consistently.
…e test suite. Also, removed the Axios mock adapter dependency completely.
node-fetch supports 6.×.× (and even 4; but not 5). However, having the version requirement at 8.×.× makes testing easier, as some testing utilities don't support older versions.
A note on Node.js version support.One could make the argument that periodically increasing the Node.js version this client requires is a good thing, because it motivates users to upgrade their Node.js version to one which is still in the maintenance window. I personally suspect and fear the true effect would instead be that users stop updating this client altogether. I therefore continue to believe the best strategy is supporting all Node.js versions which are still being maintained, as well as any older version we can reasonably support – including through polyfills. The current oldest Node.js version which is still being maintained is 18, which will be maintained until 30 April 2025. However, through node-fetch, we can easily support 14 and up. In fact, node-fetch 2.× supports Node.js all the way down to 6. However, Node.js versions under 14 are challenging to test, as Jest and Nock have version requirements of their own. We decided to raise the Node.js version requirement to 14, while secretly supporting 8. We've tested the client including this PR on Node.js 8, and it does work, but the test suite itself needs to be modified to make the tests themselves compatible. |
No description provided.