Skip to content
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

Merged
merged 23 commits into from
Sep 10, 2024
Merged

Replace axios with (node-)fetch #358

merged 23 commits into from
Sep 10, 2024

Conversation

janpaepke
Copy link
Collaborator

No description provided.

package.json Show resolved Hide resolved
@@ -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'>;
};
Copy link
Collaborator

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.

@Pimm Pimm mentioned this pull request Sep 6, 2024
@Pimm
Copy link
Collaborator

Pimm commented Sep 6, 2024

Fixes #346.

src/communication/NetworkClient.ts Outdated Show resolved Hide resolved
src/communication/NetworkClient.ts Outdated Show resolved Hide resolved
src/communication/makeRetrying.ts Outdated Show resolved Hide resolved
janpaepke added a commit to janpaepke/mollie-api-node that referenced this pull request Sep 10, 2024
janpaepke added a commit to janpaepke/mollie-api-node that referenced this pull request Sep 10, 2024
janpaepke added a commit to janpaepke/mollie-api-node that referenced this pull request Sep 10, 2024
janpaepke added a commit to janpaepke/mollie-api-node that referenced this pull request Sep 10, 2024
dsempel and others added 19 commits September 10, 2024 15:28
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.
@janpaepke janpaepke merged commit 476dc50 into mollie:master Sep 10, 2024
5 checks passed
@Pimm
Copy link
Collaborator

Pimm commented Sep 10, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants