Migrate to ofetch #904

wa0x6e · 2023-09-21T11:52:03Z

🧿 Current issues / What's wrong ?

Current fetch request does not support timeout, nor keep alive connections.

💊 Fixes / Solution

Migrate the fetch library from cross-fetch to ofetch.

The error handling changes from ofetch will also introduce a change in returned response on error.

Previously, error handling looks like:

try {
  const body = await getScores(x);
  if (body.error) { // Handle error from score-api };
  return body.result;
} catch(e) {
  // do something with e, which is an Error object, for network error, or JSON parse error
}

With ofetch changes,

try {
  const body = await getScores(x);
  return body.result;
} catch(e) {
  // do something with e, which is an JSON-RPC object
  if (typeof e.code === 'string') {
    // Error from score-api
  } else {
    // Network error 
  }
}

🚧 Changes

Add support for keep alive connections
Add timeout to all fetch requests
All non-200 status code throw an error
All errors thrown follow the same format as the underlying service error, beside getJSON and ipfsGet (see below)

`getScores`, `validate` and `getVp`

SUCCESS: The promise will resolve with the score JSON object
ERROR (from score-api): Promise rejection with JSON-RPC object (code === string)
ERROR (network): Promise rejection with JSON-RPC object (code === number)

The last argument (options) accepts a timeout property.

`subgrapherRequest` and `getSnapshot`

SUCCESS: return the JSON object
ERROR (subgrapher): return a JSON graphQL error object
ERROR (network): return a GraphQL like error object
ERROR (not json response): return a GraphQL like error object

The last argument (options) accepts a timeout property.

`getJSON` and `ipfsGet`

SUCCESS: return the JSON object
ERROR (network): throw a FetchError
ERROR (not json response): throw an JSON parse error

The last argument (options) accepts a timeout property.

`send` (from Client)

SUCCESS: return the JSON object
ERROR (from sequencer): Promise rejection with JSON-RPC object
ERROR (network): throw FetchError

‼️ Breaking changes

ofetch is handling error response differently, and is rejecting a Promise on all non-200 errors.

This new update will require a bump to the next major version (1.x.x).
As this new breaking change need more testing before public release, the new 1.x.x should be released as a beta, and used only by our internal services first (and keep updated with the main branch) for a few weeks.

Once everything confirmed as working as expected, then we can public release it.

🛠️ Tests

Call the getScores, validate, getSnapshot and getVp functions with valid data
It should return the expected object
Call them again with invalid data
It should return a JSON-RPC error object, from score-api
Call them again with invalid url (pass something else as score-api url)
It should return JSON-RPC error object

codecov · 2023-09-21T14:51:55Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (350a1ef) 44.19% compared to head (bec2e3a) 72.15%.

❗ Current head bec2e3a differs from pull request most recent head cfb0e3e. Consider uploading reports for the commit cfb0e3e to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #904       +/-   ##
===========================================
+ Coverage   44.19%   72.15%   +27.96%     
===========================================
  Files          22       22               
  Lines        2100     2144       +44     
  Branches      195      249       +54     
===========================================
+ Hits          928     1547      +619     
+ Misses       1162      590      -572     
+ Partials       10        7        -3

Files	Coverage Δ
src/sign/index.ts	`54.66% <100.00%> (+54.66%)`	⬆️
src/utils.ts	`61.60% <99.14%> (+26.06%)`	⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/utils.ts

…pshot.js into migrate-ofetch

Todmy

utACK

Todmy

utACK

wa0x6e · 2023-09-22T16:01:04Z

Fixed the fetch request from Client, please review again, as it's a core function

package.json

src/sign/index.ts

Todmy

utACK

src/sign/index.ts

bonustrack · 2023-09-26T10:02:11Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "@snapshot-labs/snapshot.js",
-  "version": "0.6.2",
+  "version": "1.0.0-beta.0",


Why this change? We shouldn't go from a normal version to a beta. Also not sure we should bump to v1. I dont see any change in function or parameters

Pretty major change in error handling from ofetch, as pointed out in yesterday call. Beta will be released and tested only interbally first, and not for public

bonustrack · 2023-09-26T10:03:18Z

@Todmy Can you test this PR, utACK is not enough

Todmy · 2023-09-29T09:45:52Z

@wa0x6e could you please test your tests? I run them and have some failed

a lot of them related to the https://httpstat.us/200?sleep=5000 but there are other errors

Todmy

tests should be fixed

wa0x6e · 2023-09-29T16:28:26Z

@wa0x6e could you please test your tests? I run them and have some failed a lot of them related to the https://httpstat.us/200?sleep=5000 but there are other errors

Tests are passing on my local, and on github actions 🤔

What are the errors you're seeing ?

wa0x6e · 2023-09-29T18:07:08Z

Added retry to flaky tests depending on third party services

Todmy · 2023-10-02T10:12:59Z

issue persists. The reason why tests fail on my local host is that I use node@18. This package is used for projects with node@18 as well.

wa0x6e · 2023-10-02T10:14:41Z

Hmmmm… have to start adding node 18 to ci

wa0x6e · 2023-10-02T16:31:26Z

Hmmmm… have to start adding node 18 to ci

#911

wa0x6e · 2023-10-03T02:03:57Z

issue persists. The reason why tests fail on my local host is that I use node@18. This package is used for projects with node@18 as well.

Tests fixed for node 18, also confirmed to work on node 20

Todmy

tACK

test/e2e/utils/blockfinder.spec.js

wa0x6e · 2023-10-05T18:12:13Z

I believe this should not be merged on master, but on a dedicated v1 branch. We could then npm publish the beta from that branch

ChaituVR · 2023-10-06T18:17:47Z

Maybe we can leave it on this branch, can i publish it to beta tag?

wa0x6e · 2023-10-06T18:36:12Z

Maybe we can leave it on this branch, can i publish it to beta tag?

I don’t think it’s possible to have 2 versions on the same branch, it’ll create a merge conflict

ChaituVR · 2023-10-09T05:38:50Z

Published this branch to https://www.npmjs.com/package/@snapshot-labs/snapshot.js/v/0.8.0-beta.0

wa0x6e added 4 commits September 21, 2023 02:08

chore: migrate fetch lib from cross-fetch to ofetch

1c2f3d9

feat: add timeout to fetch requests

0304356

chore: fix tests

60faabe

Merge branch 'master' into migrate-ofetch

bf0c869

wa0x6e marked this pull request as ready for review September 21, 2023 14:51

wa0x6e requested review from ChaituVR and Todmy September 21, 2023 14:51

wa0x6e changed the title ~~Migrate ofetch~~ Migrate to ofetch Sep 21, 2023

wa0x6e added 2 commits September 22, 2023 00:03

chore: add more tests

170b932

chore: add more test

b212168

Todmy reviewed Sep 21, 2023

View reviewed changes

src/utils.ts Outdated Show resolved Hide resolved

src/utils.ts Outdated Show resolved Hide resolved

wa0x6e added 4 commits September 22, 2023 17:16

Merge branch 'master' into migrate-ofetch

a910087

chore: remove uneeded JSON.stringify

3dc29cb

Merge branch 'migrate-ofetch' of https://github.com/snapshot-labs/sna…

6bc962f

…pshot.js into migrate-ofetch

chore: split long line

77a6c2f

Todmy approved these changes Sep 22, 2023

View reviewed changes

fix: handle errors when parsing json response

cfd5246

Todmy approved these changes Sep 22, 2023

View reviewed changes

fix: migrate fetch request to ofetch API

fc18bab

wa0x6e requested a review from Todmy September 22, 2023 16:00

Todmy suggested changes Sep 25, 2023

View reviewed changes

package.json Show resolved Hide resolved

src/sign/index.ts Show resolved Hide resolved

chore: bump version

836dddf

wa0x6e requested a review from Todmy September 25, 2023 15:00

wa0x6e mentioned this pull request Sep 25, 2023

Fetch with timeout #854

Closed

Todmy approved these changes Sep 26, 2023

View reviewed changes

src/sign/index.ts Show resolved Hide resolved

bonustrack reviewed Sep 26, 2023

View reviewed changes

Todmy self-requested a review September 26, 2023 10:25

chore: fix tests

3bb41c9

wa0x6e requested review from Todmy and bonustrack September 28, 2023 17:21

Todmy suggested changes Sep 29, 2023

View reviewed changes

wa0x6e added 4 commits September 30, 2023 02:05

chore: remove redundant function calls

3d1dc2a

chore: fix test

dbc44e0

chore: add retry to flaky tests

261a6e4

chore: improve tests

58fc436

wa0x6e requested a review from Todmy September 29, 2023 18:07

Merge branch 'master' into migrate-ofetch

520b95e

wa0x6e mentioned this pull request Oct 2, 2023

chore: run CI tests on more node versions #911

Merged

wa0x6e added 2 commits October 3, 2023 01:32

Merge branch 'master' into migrate-ofetch

96d7df1

chore: fix tests for node 18

93f8069

Todmy approved these changes Oct 3, 2023

View reviewed changes

ChaituVR reviewed Oct 5, 2023

View reviewed changes

test/e2e/utils/blockfinder.spec.js Outdated Show resolved Hide resolved

Merge branch 'master' into migrate-ofetch

bec2e3a

wa0x6e added 2 commits October 9, 2023 14:13

Merge branch 'master' into migrate-ofetch

b2cc4b2

chore: bump feat version instead of major

cfb0e3e

IndigoFlo approved these changes Apr 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to ofetch #904

Migrate to ofetch #904

wa0x6e commented Sep 21, 2023 •

edited

Loading

codecov bot commented Sep 21, 2023 •

edited

Loading

Todmy left a comment

Todmy left a comment

wa0x6e commented Sep 22, 2023

Todmy left a comment

bonustrack Sep 26, 2023

wa0x6e Sep 26, 2023

bonustrack commented Sep 26, 2023

Todmy commented Sep 29, 2023

Todmy left a comment

wa0x6e commented Sep 29, 2023

wa0x6e commented Sep 29, 2023

Todmy commented Oct 2, 2023

wa0x6e commented Oct 2, 2023

wa0x6e commented Oct 2, 2023

wa0x6e commented Oct 3, 2023 •

edited

Loading

Todmy left a comment

wa0x6e commented Oct 5, 2023

ChaituVR commented Oct 6, 2023

wa0x6e commented Oct 6, 2023

ChaituVR commented Oct 9, 2023

Migrate to ofetch #904

Are you sure you want to change the base?

Migrate to ofetch #904

Conversation

wa0x6e commented Sep 21, 2023 • edited Loading

🧿 Current issues / What's wrong ?

💊 Fixes / Solution

🚧 Changes

getScores, validate and getVp

subgrapherRequest and getSnapshot

getJSON and ipfsGet

send (from Client)

‼️ Breaking changes

🛠️ Tests

codecov bot commented Sep 21, 2023 • edited Loading

Codecov Report

Todmy left a comment

Choose a reason for hiding this comment

Todmy left a comment

Choose a reason for hiding this comment

wa0x6e commented Sep 22, 2023

Todmy left a comment

Choose a reason for hiding this comment

bonustrack Sep 26, 2023

Choose a reason for hiding this comment

wa0x6e Sep 26, 2023

Choose a reason for hiding this comment

bonustrack commented Sep 26, 2023

Todmy commented Sep 29, 2023

Todmy left a comment

Choose a reason for hiding this comment

wa0x6e commented Sep 29, 2023

wa0x6e commented Sep 29, 2023

Todmy commented Oct 2, 2023

wa0x6e commented Oct 2, 2023

wa0x6e commented Oct 2, 2023

wa0x6e commented Oct 3, 2023 • edited Loading

Todmy left a comment

Choose a reason for hiding this comment

wa0x6e commented Oct 5, 2023

ChaituVR commented Oct 6, 2023

wa0x6e commented Oct 6, 2023

ChaituVR commented Oct 9, 2023

wa0x6e commented Sep 21, 2023 •

edited

Loading

`getScores`, `validate` and `getVp`

`subgrapherRequest` and `getSnapshot`

`getJSON` and `ipfsGet`

`send` (from Client)

codecov bot commented Sep 21, 2023 •

edited

Loading

wa0x6e commented Oct 3, 2023 •

edited

Loading