Fixes #86 - Pluggable Fetch #109

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/[email protected]
=== 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

                    timeoutHandler = setTimeout(() => controller.abort(), retryOptions.socketTimeout);
                    options.signal = controller.signal;
                }                

                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

@@ -852,9 +841,20 @@ describe('test fetch retry', () => {
        const response = await fetch(`${FAKE_BASE_URL}${FAKE_PATH}`);
        assert.strictEqual(response.ok, true);
    });
+}
+
+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

        assert.strictEqual(response.status, 200);
    });
+}
+
+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

    it('test fetch get works 200', async () => {
        nock(FAKE_BASE_URL)
            .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)

klcodanr 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 commented Jan 19, 2023

adobe-bot commented Jan 19, 2023

codecov bot commented Jan 19, 2023 •

edited

Loading

jdelbick Jan 19, 2023

jdelbick Jan 19, 2023

jdelbick Jan 19, 2023

jdelbick Jan 19, 2023

jdelbick Jan 19, 2023

jdelbick Jan 19, 2023

jdelbick left a comment •

edited

Loading

jdelbick Jan 19, 2023

	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', () => {

Fixes #86 - Pluggable Fetch #109

Are you sure you want to change the base?

Fixes #86 - Pluggable Fetch #109

Conversation

klcodanr commented Jan 19, 2023

Description

Types of changes

Checklist:

adobe-bot commented Jan 19, 2023

codecov bot commented Jan 19, 2023 • edited Loading

Codecov Report

jdelbick Jan 19, 2023

Choose a reason for hiding this comment

jdelbick Jan 19, 2023

Choose a reason for hiding this comment

jdelbick Jan 19, 2023

Choose a reason for hiding this comment

jdelbick Jan 19, 2023

Choose a reason for hiding this comment

jdelbick Jan 19, 2023

Choose a reason for hiding this comment

jdelbick Jan 19, 2023

Choose a reason for hiding this comment

jdelbick left a comment • edited Loading

Choose a reason for hiding this comment

jdelbick Jan 19, 2023

Choose a reason for hiding this comment

codecov bot commented Jan 19, 2023 •

edited

Loading

jdelbick left a comment •

edited

Loading