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

feat: remove graphql-request dependency #1597

Closed
wants to merge 118 commits into from

Conversation

nedsalk
Copy link
Contributor

@nedsalk nedsalk commented Dec 31, 2023

This PR replaces the graphql-request and graphql-tag dependencies with a simple fetch implementation.

This change reduces the final bundle size by 15.68 kB.

arboleya and others added 30 commits December 8, 2023 16:43
* docs: purge hardcoded snippets on 'using typegen' page

* Update .gitignore

* delete typegen files

* fix formatting

* update ignore files

* refactor package.json

* fix author

Co-authored-by: Cameron Manavian <cameron.manavian@fuel.sh>

* fix author

Co-authored-by: Cameron Manavian <cameron.manavian@fuel.sh>

* update predicate test

* fix failing predicate test

* Fix authors

* add gasLimit

* update gas limit

---------

Co-authored-by: Cameron Manavian <cameron.manavian@fuel.sh>
Co-authored-by: Daniel Bate <djbate23@gmail.com>
* Revert "feat: add `Predicate.getTransferTxId` helper (#1467)"

This reverts commit 70233c1.

* chore: add rc workflow

* chore: changeset

* chore: change rc workflow name

* chore: fix rc version

* chore: remove workflow dispatch for rc worjflow

* chore: add correct rc naming

* chore: use correct branch filtering

* chore: linting

* chore: alter branch name env

* chore: change pull request to push

* feat: set correct rc versiuon in worflow

* feat: add echo rc verison

* chore: test ci

* chore: test ci

* chore: echo changesets

* chore: use env in rc changeset

* test dan release

* chore: enable pr release

* chore: copy pr relase

* try rc nam,e

* use salamander

* chore: use rc name

* use: salamander

* chore: test salamander

* chore: fix workflow trigger

* chore: remove env rc name

* chore: update rc suffix

---------

Co-authored-by: Anderson Arboleya <anderson@arboleya.me>
* chore: fix string replace

* chore: changeset
…1520)

* docs: Updated high-level doc links in README files

* docs: updating "/guide/" based doc hyperlinks

* docs: update "/sway/" and "/forc/" related doc urls

* docs: update "fuels-rs" doc links

* docs: update quickstart doc links

* docs: updated the quickstart docs link

* docs: added the changeset

---------

Co-authored-by: Anderson Arboleya <anderson@arboleya.me>
* chore: pin graphql request version

* chore: changeset
* chore: upgrade tsx

* chore: remove redundant script

* chore: changeset
* Setting up

* Adjusting tests from `packages/address`

* Adjusting tests from `packages/abi-typegen`

* Forgotten files

* Adjusting scripts

* Touchups

* Adjusting script

* Touchups

* Adjusting tests for `packages/crypto`

* Adjusting tests for `packages/errors`

* feat: implement coverage merging, reporting. Also in test workflow with test validation

* feat: migration of functionalities from vitest

* feat: further config change to implement vitest

* feat: some jest -> vitest syntax migration

* feat: migrate jest spys to vite

* feat: add todo vitest mocks

* feat: upgrade lodash and ethers in hasher package

* feat: install buffer for tests

* feat: add vite env and browser config

* chore: lint

* feat: add missing test groups

* feat: implement check tests module

* chore: update lock file

* feat: add missing test env tags

* chore: regen lock file

* feat: migrate jest mocks to vite in providers

* feat: add ts expect error for browser test for browser apis

* feat: add missing vite restore mocks to provider test

* feat: replace provider mocks with vite compatible mocks

* feat: reenable env specific tests

* feat: enable ts dom lib

* efat: remove ts expect error for browser apis

* feat: fallback to first faucet for vite env

* feat: move vite config files to ts

* feat: polyfill node apis

* feat: uninstall buffer in place of browser polyfill

* feat: remove node process import from vite env

* feat: remove vi import from vite env

* feat: example commit with hasher hardlinked

* chore: regen lock file

* feat: migrate all jest mocking and fix all node tests

* chore: remove redundant snapshots

* feat: add ts expect error for vite mocks

* feat: add node group tags to all tests

* feat: symlink fuels hasher deps

* chore: regen lock file

* feat: fix test groups

* feat: remove redundant process dep

* feat: migrate to vite hooks in typegen test

* feat: alter coverage reporters

* feat: add missing test group

* feat: reduce pnpm test commands and simplify test scripts

* feat: remove browser test stage from test workflow

* feat: implement browser test command

* feat: alter docs that reference jest

* chore: changeset

* feat: fix doc snippets

* feat: don't run rewrite scripts inside vitest

* feat: fix doc snippet tags

* feat: add missing tsdoc files

* feat: remove redundant imports

* chore: linting

* feat: remove workflow from download artifact action in gh workflow

* feat: alter test workflow

* feat: add support for live node tests in vitest

* feat: use different workflow download action

* feat: fix check tests to find all tests

* feat: migrate jest to vi in provider tests

* feat: fix provider mock in provider unit test

* feat: add e2e to all find tests flag

* feat: remove process define in place of polyfill plugin

* feat: rename tests coverage script functions

* feat: fix vitest mock in mock versions test

* feat: reintroduce hasher tests

* feat: update test contribution docs

* feat: rename coverage commands

* feat: add missing test groups

* chore: force rebuild

* feat: add test and coverage exclusions

* feat: reitroduce coverage flags

* feat: add missing test group

* feat: remove file from master merge

* feat: use more updated lcov workflow action

* chore: rebuild

* feat: use old coverage report action

* feat: move test CI setup to a composite action

* feat: reimplement checkout to test CI

* feat: add shell to test setup CI action

* chore: force rebuild

* feat: fix test ci

* feat: create test setup action

* feat: implement coverage diff script and PR comment action

* feat: resolve issue in previous merge conflict

* feat: add missing test envs

* feat: remove temp test ci stage

* feat: alter find tests script to produce new diff

* feat: alter find tests script to produce new diff

* feat: recreate coverage PR comment

* feat: fix find test script

* Adjusting new tests (merged from master) to vite

* Adding missing test’s group tag

* chore: regen lock file

* feat: restore contents of crypto

* feat: fix crypto test mocks

* feat: fix shell js refactors

* feat: implement filter tests command

* feat: isolate browser tests

* feat: migrate test validate script to sh

* feat: remove browser test stages from test workflow

* feat: rename gen coverage script to merge coverage

* feat: rollback crypto types change

* feat: fix browser types

* feat: remove redundant test workflow step

* chore: linting

* feat: add missing crypto tsdoc file

* feat: improve test ci stage names

* feat: remove test that was brought in merge

* feat: migrate fuels cli work to vitest

* chore: linting

* feat: implement test matrix to speed up builds

* feat: rename test matrix stage

* chore: linting

* Moving fixtures around

* Revamping test utilities

* Updating gitignore

* Re-stitching files for `build` command

* Re-stitching files for `dev` command

* Re-stitching core cli files

* Fixing broken mocks

* Refactoring all feature tests

* Fixing all broken unit tests

* Adding missing units

* Re-enabling convenient html report generation

* Re-position flag appropriately

* Ensuring method interruption

* Setting up cleanup hook

* Removing useless method call

* Lintfix

* Adjusting hook timing

* Adding [missing] test group tag

* Adjusting mocks’ restoration

* Lintfix

* Fixing tsconfig

* Fixing test mocks

* Lintfix

* Experimenting with a bigger timeout on long-running tests

* Removing unused import

* chore: force rebuild

* feat: rename test ci stages

* chore: update changeset

Co-authored-by: Nedim Salkić <nedim.salkic@fuel.sh>

* feat: change test workflow stage names

* feat: reintroduce test step

* feat: improve diff text

* feat: add missing test grouops

* chore: spelling

* chore: fix unused imports

* chore: linting

* chore: upgrade vite, vitest and deps

* chore: remove unused ts expect errors

* test: fix import

* test: update hbs assertion

* test: fix forc project mocks

* test: fix hbs assertion

* test: fix ethers mock

* test: add missing test groups

* feat: ignore apps/docs in vite

* chore: update lock

* test: update math mock in provider test

* test: fix math mock

* chore: update lock

* chore: rebuild

* chore: update lock

* chore: pin graphql request version

* chore: regen lock

* chore: migrate to vite mts

* test: resolve fuels mocks

* test: add missing group

* chore: DRY build sway programs test

* test: remve invalid jest calls

* chore: rebuild

* chore: upgrade vite deps

* chore: allocate more memory to test workflow

* chore: rebuild

* chore: add nyc excluded

* chore: increase exclusions

* chore: alter merge report

* chore: alter test runner

* chore: add coverage includes

* chore: try without max mem size in CI

* chore: rebuild

---------

Co-authored-by: Anderson Arboleya <anderson@arboleya.me>
Co-authored-by: Daniel Bate <--global>
Co-authored-by: Nedim Salkić <nedim.salkic@fuel.sh>
* chore: changeset

* chore: use int array for temp stage

* chore: fix conditional workflow
* chore: update node engine in create fuels

* chore: changeset
Base automatically changed from rc/salamander to master January 26, 2024 15:06
@arboleya arboleya modified the milestones: 1 - Salamander, 3 - Wasp Jan 31, 2024
@nedsalk
Copy link
Contributor Author

nedsalk commented Feb 2, 2024

1 - Comprehensive Error Handling: The graphql-request library provides a robust mechanism for handling various types of errors, which can be crucial in a production environment.

I went through their source code and didn't find anything comprehensive around error handling. I only found them providing the ability to configure an error policy, which is something we don't have a need for because we want to throw all errors.

While I understand that in this proposed solution, any thrown error will be caught elsewhere, there is a clear benefit to having specific errors properly handled and managed within the context of the library itself.

The only specific error they have is ClientError, meaning they just differentiate between two types of errors:

  • Those that are returned by the client in the response body's errors property (handled by their ClientError),
  • all other errors that are just re-thrown

They don't provide any comprehensive error handling because that's not their purpose as they're a general-purpose library. It is up to their consumers to do a try/catch on their ClientError, read the message or response field, and then differentiate between specific errors by implementing custom error handling via some string matching. This PR isn't passing the error-handling responsibility away to someone else, it's doing the exact same amount of error handling as does graphql-request.

2 - Subscription Management: graphql-request effectively manages multiple subscriptions, which is important for resource management.

graphql-request only supports subscriptions over web sockets, which isn't applicable to us as our fuel node implements subscriptions over SSE.

3 - Community Support and Maintenance: By utilizing a well-established library like graphql-request, we benefit from ongoing community support and regular updates. This significantly reduces the maintenance burden on our team and ensures that we stay aligned with industry standards and best practices.

This library has been in the SDK's dependencies since the beginning and its only usage was for creating a proper fetch request and parsing the response correctly - something that was replaced by 21 lines of code (create a request object and send and parse the response). We don't need the ceremonies of a library to do a simple fetch, however well the library is maintained. The maintenance burden of a fetch call is minimal and IMO less than keeping up with a library's changes in this simple context. Any industry standard improvements upon fetch will benefit both us and graphql-request the same.

I suggest we carefully consider the long-term implications of this change. It might be more beneficial to continue using graphql-request, ensuring that we have a robust, well-supported solution for our GraphQL needs.

IMO two-ish years of usage in the SDK that only amounted to the use case of fetching is enough to say, "We don't need this library if we can replace it with a simple fetch". Fetching has been our only need met by graphql-request. IMO fetch should have been the default here since the beginning and a library like graphql-request should've been considered only if some additional functionality was needed.

As far as the future goes, if the potential case arises where we need some additional functionality we can always take inspiration from them or even return to them if we see that it's too complex to maintain. As for the current situation, the size reduction and easier grokking of the whole 21-line fetching functionality compared to whatever other library that hides it internally IMO is enough to go through with this PR.

Copy link
Contributor

github-actions bot commented Feb 5, 2024

Coverage Report:

Lines Branches Functions Statements
78.29%(-0.13%) 68.52%(+0.09%) 76.47%(-0.18%) 78.26%(-0.13%)
Changed Files:
Ok File (✨=New File) Lines Branches Functions Statements
🔴 ✨ packages/providers/src/fuel-graphql-request.ts 97.77%
(+97.77%)
95%
(+95%)
100%
(+100%)
97.82%
(+97.82%)
🔴 packages/providers/src/fuel-graphql-subscriber.ts 0%
(-80%)
0%
(-37.5%)
0%
(-100%)
0%
(-80%)
🔴 packages/providers/src/provider.ts 70.27%
(-0.67%)
55.46%
(-0.73%)
66.66%
(-1.03%)
68.79%
(-0.8%)

@Dhaiwat10
Copy link
Member

Dhaiwat10 commented Feb 17, 2024

My two cents on this PR:

If everything is working as intended like @Torres-ssf mentioned, I think we should go ahead with this PR and get it merged.

I did read through the points that Sergio mentioned in his comment, too. I think the Nedim's explanation in his response is fair enough. I don't see a downgrade in terms of quality/reliability or an increase in burden maintenance-wise if we go ahead with Nedim's solution. On top of that, this PR would decrease the bundle size - which is crucial.

I agree that we cannot be 100% sure that nothing will break unexpectedly, but we can be 99% sure and that's good enough. If something pops up, we can put together a quick fix or roll things back. The benefits outweigh the costs here.

@arboleya
Copy link
Member

All points make sense, but I'm mainly concerned with unknown unknowns and the possible introduction of unforeseen problems, especially in preparation for the subsequent releases, as I'm not sure the risk outweighs the benefits.

@danielbate
Copy link
Contributor

I agree with the fact that graphql-request was potentially overkill at the time it was implemented here. As it was implemented at the inception of the SDK, it was likely it was chosen to obtain functionality with minimal overhead. Now we have the time to question these decisions and I am glad we are doing so. I believe a fetch would suffice, if we needed additional functionality that we don't want to maintain, at that point we'd bring in a library. The using a sledgehammer to crack a nut analogy comes to mind here.

My main problem here is that right now stability should be our top priority, alongside delivering main net features. Although package size is a big consideration, I would rather deliver a stable package that we have complete confidence in than one that is leaner in size, but has unknowns. At this point in time, if something even has 1% unknowns it is worth not merging at the moment. Especially, as we already have a working solution. I'd be in favour of closing the PR.

@nedsalk
Copy link
Contributor Author

nedsalk commented Feb 19, 2024

Closing this PR because most of the team is not in favor of it at the moment.

@nedsalk nedsalk closed this Feb 19, 2024
@nedsalk

This comment was marked as outdated.

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

Successfully merging this pull request may close these issues.

Remove graphql-request dependency
6 participants