Fixes #86 - Pluggable Fetch by klcodanr · Pull Request #109 · adobe/node-fetch-retry

klcodanr · 2023-01-19T20:28:37Z

Description

Adding support for plugging the fetch implementation and verifying with @adobe/fetch

Fixes # (issue)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

…verifying with @adobe/fetch

adobe-bot · 2023-01-19T20:29:35Z

🎉 Sizewatcher congratulates on the size improvement 📉:

🎉 git -75.5% (707 kB => 173 kB)

✅ node_modules +18.1% (74.8 MB => 88.3 MB)

Largest production node modules:

┌──────────────────┬────────────┬───────┐
│ name             │ children   │ size  │
├──────────────────┼────────────┼───────┤
│ @adobe/fetch     │ 0          │ 0.25M │
├──────────────────┼────────────┼───────┤
│ node-fetch       │ 0          │ 0.15M │
├──────────────────┼────────────┼───────┤
│ abort-controller │ 0          │ 0.07M │
├──────────────────┼────────────┼───────┤
│ 3 modules        │ 0 children │ 0.47M │
└──────────────────┴────────────┴───────┘

✅ npm_package +0.9% (11.5 kB => 11.6 kB)

Package contents:

📦  @adobe/node-fetch-retry@2.2.0
=== Tarball Contents === 
850B   .releaserc.json   
3.2kB  CODE_OF_CONDUCT.md
171B   COPYRIGHT         
11.3kB LICENSE           
7.6kB  README.md         
555B   index.d.ts        
12.7kB index.js          
1.2kB  package.json      
=== Tarball Details === 
name:          @adobe/node-fetch-retry                 
version:       2.2.0                                   
filename:      @adobe/node-fetch-retry-2.2.0.tgz       
package size:  11.6 kB                                 
unpacked size: 37.6 kB                                 
shasum:        a0184e838af007f31636ada11f7a87b190eeac36
integrity:     sha512-hU8UWUzLYw941[...]/vW4rxXdM0+VQ==
total files:   8                                       

npm WARN This command requires you to be logged in to https://registry.npmjs.org/ (dry-run)
Publishing to https://registry.npmjs.org/ (dry-run)

Notes

PR branch: feature/86-adobe-fetch-support @ 2a3df64
Base branch: master
Sizewatcher v1.2.1
Effective Configuration:

limits:
  fail: 100%
  warn: 30%
  ok: '-10%'
report:
  githubComment: true
  githubStatus: false
comparators: {}

codecov · 2023-01-19T20:30:32Z

Codecov Report

Merging #109 (2a3df64) into master (126d3c4) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 2a3df64 differs from pull request most recent head 930a3e6. Consider uploading reports for the commit 930a3e6 to get more accurate results

@@            Coverage Diff             @@
##           master     #109      +/-   ##
==========================================
+ Coverage   98.87%   98.90%   +0.02%     
==========================================
  Files           1        1              
  Lines          89       91       +2     
==========================================
+ Hits           88       90       +2     
  Misses          1        1

Impacted Files	Coverage Δ
index.js	`98.90% <100.00%> (+0.02%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

jdelbick · 2023-01-19T20:36:46Z

README.md

+You can disable all retry behavior by setting `retryOptions` to `false`.
+
+```js
+const {fetch: adobeFetch, AbortController} = require('@adobe/fetch');


what is the difference between adobe/fetch's abort controller and ours?

jdelbick · 2023-01-19T20:43:05Z

index.js


                try {
-                    const response = await fetch(url, options);
+                    const fetchFn = options.fetch || fetch;


this recursively invokes wrappedFetch(); so i would prefer for you to set the fetchFn once if possible. (somehow globally maybe).
Maybe there is a way to declare on import which fetch to use?

In case the options may get wiped away between retries (maybe this is too unrealistic)

jdelbick · 2023-01-19T20:49:36Z

package.json

-    "rewire": "^5.0.0",
-    "semantic-release": "^17.4.7"
+    "rewire": "^6.0.0",
+    "semantic-release": "^20.0.2"


typically we will update major versions of dev dependencies in follow-up PRs in case they require additional changes. For instance sometimes semantic-release updates require nodejs verion increases and we won't notice that until we actually try to release, (happened before, had to update to use nodejs 16: https://github.com/adobe/node-fetch-retry/blob/master/.github/workflows/node.js.yml#L47)

ah yep, nodejs18 is required for v20+: https://github.com/semantic-release/semantic-release/releases/tag/v20.0.0

jdelbick · 2023-01-19T20:54:07Z

test/index.test.js

    });
+}
+
+describe('test fetch retry', () => testFetchRetry({}));


Suggested change

describe('test fetch retry', () => testFetchRetry({}));

describe('test fetch retry with default node-fetch', () => testFetchRetry({}));

jdelbick · 2023-01-19T20:54:28Z

test/index.test.js

    });
+}
+
+describe('test fetch retry on http errors (throw exceptions)', () => {


Suggested change

describe('test fetch retry on http errors (throw exceptions)', () => {

describe('test fetch retry on http errors (throw exceptions) with default node-fetch', () => {

jdelbick

Please check the semantic release changes won't break the CI (or just remove them), other than that LGTM

jdelbick · 2023-01-19T20:59:45Z

test/index.test.js

            .get(FAKE_PATH)
            .reply(200, { ok: true });
-        const response = await fetch(`${FAKE_BASE_URL}${FAKE_PATH}`, { method: 'GET' });
+        const response = await fetch(`${FAKE_BASE_URL}${FAKE_PATH}`, { method: 'GET', ...options });


super nitpick test case i just thought of, add a test without passing ,...options just to make sure it works as before. (I know we default options to {}:

node-fetch-retry/index.js

Line 200 in 930a3e6

options = options || {};

, so this test would serve as a safeguard in case it gets removed)

Dan Klco added 2 commits January 19, 2023 15:24

Fixes #86 - Adding support for plugging the fetch implementation and …

3fab3f9

…verifying with @adobe/fetch

Adding documentation for providing fetch

930a3e6

klcodanr requested a review from jdelbick January 19, 2023 20:28

jdelbick approved these changes Jan 19, 2023

View reviewed changes

jdelbick requested changes Jan 19, 2023

View reviewed changes

jdelbick reviewed Jan 19, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #86 - Pluggable Fetch#109

Fixes #86 - Pluggable Fetch#109
klcodanr wants to merge 2 commits intomasterfrom
feature/86-adobe-fetch-support

klcodanr commented Jan 19, 2023

Uh oh!

adobe-bot commented Jan 19, 2023

Uh oh!

codecov bot commented Jan 19, 2023 •

edited

Loading

Uh oh!

jdelbick Jan 19, 2023

Uh oh!

jdelbick Jan 19, 2023

Uh oh!

jdelbick Jan 19, 2023

Uh oh!

jdelbick Jan 19, 2023

Uh oh!

jdelbick Jan 19, 2023

Uh oh!

jdelbick Jan 19, 2023

Uh oh!

jdelbick left a comment •

edited

Loading

Uh oh!

jdelbick Jan 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	describe('test fetch retry', () => testFetchRetry({}));
	describe('test fetch retry with default node-fetch', () => testFetchRetry({}));

	describe('test fetch retry on http errors (throw exceptions)', () => {
	describe('test fetch retry on http errors (throw exceptions) with default node-fetch', () => {

Conversation

klcodanr commented Jan 19, 2023

Description

Types of changes

Checklist:

Uh oh!

adobe-bot commented Jan 19, 2023

Uh oh!

codecov bot commented Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jdelbick Jan 19, 2023

Choose a reason for hiding this comment

Uh oh!

jdelbick Jan 19, 2023

Choose a reason for hiding this comment

Uh oh!

jdelbick Jan 19, 2023

Choose a reason for hiding this comment

Uh oh!

jdelbick Jan 19, 2023

Choose a reason for hiding this comment

Uh oh!

jdelbick Jan 19, 2023

Choose a reason for hiding this comment

Uh oh!

jdelbick Jan 19, 2023

Choose a reason for hiding this comment

Uh oh!

jdelbick left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdelbick Jan 19, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jan 19, 2023 •

edited

Loading

jdelbick left a comment •

edited

Loading