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: GraphQL subscriptions #1374

Merged
merged 77 commits into from
Dec 20, 2023
Merged

feat: GraphQL subscriptions #1374

merged 77 commits into from
Dec 20, 2023

Conversation

nedsalk
Copy link
Contributor

@nedsalk nedsalk commented Oct 25, 2023

This PR introduces GraphQL subscriptions without a dependency on any external library by using the Streams API and async generators. The native EventSource unfortunately couldn't cut it because it's limited only to GET requests. Alternatives to EventSource like eventsource, SSE.js, fetch-event-source have their own problems (only GET, slow/non-existant maintenance, missing typescript declarations, lack of nodejs support...), so I opted to just use the native APIs. The core logic is in fuel-graphql-subscriber.ts and fits in less than 100 lines of code.

Also, for documentation purposes, we already tried graphql-sse and reverted it.

closes #301 #782 #1293.

@nedsalk nedsalk self-assigned this Oct 25, 2023
@nedsalk nedsalk changed the title feat: GraphQL subsriptions feat: GraphQL subscriptions Oct 25, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 25, 2023

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements
87.36% (+0.35% 🔼)
6128/7015
🟡 Branches
71.42% (+0% 🔼)
957/1340
🟡 Functions
79.15% (+1.4% 🔼)
1010/1276
🟢 Lines
87.19% (+0.18% 🔼)
5880/6744
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / fuel-graphql-subscriber.ts
82.61% 60% 100% 82.61%

Test suite run success

1483 tests passing in 259 suites.

Report generated by 🧪jest coverage report action from 2f6f236

@nedsalk nedsalk marked this pull request as ready for review October 25, 2023 15:50
@nedsalk nedsalk enabled auto-merge (squash) December 18, 2023 13:59
Torres-ssf
Torres-ssf previously approved these changes Dec 18, 2023
Copy link
Contributor

@Torres-ssf Torres-ssf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@nedsalk Why not adapt TransactionResponse class to make it possible to use the subscription submitAndAwait instead of just submit mutation when submitting a Transaction?

const {
submit: { id: transactionId },
} = await this.operations.submit({ encodedTransaction });
const response = new TransactionResponse(transactionId, this);

@nedsalk
Copy link
Contributor Author

nedsalk commented Dec 19, 2023

@Torres-ssf

Why not adapt TransactionResponse class to make it possible to use the subscription submitAndAwait instead of just submit mutation when submitting a Transaction?

The TransactionResponse class isn't concerned with submitting a transaction; it's concern is to get a submitted transaction, so I'm not sure why and how we'd adapt it for those mutations.

If you were thinking about modifying the Provider.sendTransaction method to use submitAndAwait as the default, that wouldn't be appropriate because a transaction can take an indefinite amount of time to execute on the node and using submitAndAwait as the default would block our users from executing anything on their side until the node executes the transaction.
We'd have to create a new workflow for those who want to both submit and await the execution of a transaction in one go.

@arboleya
Copy link
Member

We can have it as a secondary option, like sendTransactionAndAwait() or similar.

Copy link
Member

@arboleya arboleya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a minor observation, and there's the sendTransactionAndAwait thing.

packages/providers/test/provider.test.ts Outdated Show resolved Hide resolved
Co-authored-by: Anderson Arboleya <anderson@arboleya.me>
@nedsalk
Copy link
Contributor Author

nedsalk commented Dec 19, 2023

@arboleya

... and there's the sendTransactionAndAwait thing.

This PR is just to introduce the subscription mechanism and verify that it works on TransactionResponse by replacing the previous polling mechanism with the new server-sent events one. Introducing submitAndAwait properly for the user to benefit from it would require some additional refactoring, thinking about the API on the level above the Provider (e.g. how to best provide it on FunctionInvocationScope, Account methods like transfer, etc.), which IMO deserves to be its own PR.

@arboleya
Copy link
Member

@nedsalk I thought it'd be easy; maybe I got something wrong.

Could you please elaborate on the rationale in a new issue and link it here?

@nedsalk
Copy link
Contributor Author

nedsalk commented Dec 19, 2023

@arboleya

Could you please elaborate on the rationale in a new issue and link it here?

Here's the issue: #1563.

@arboleya
Copy link
Member

Thank you.

@Torres-ssf
Copy link
Contributor

Torres-ssf commented Dec 19, 2023

@Torres-ssf

Why not adapt TransactionResponse class to make it possible to use the subscription submitAndAwait instead of just submit mutation when submitting a Transaction?

The TransactionResponse class isn't concerned with submitting a transaction; it's concern is to get a submitted transaction, so I'm not sure why and how we'd adapt it for those mutations.

If you were thinking about modifying the Provider.sendTransaction method to use submitAndAwait as the default, that wouldn't be appropriate because a transaction can take an indefinite amount of time to execute on the node and using submitAndAwait as the default would block our users from executing anything on their side until the node executes the transaction. We'd have to create a new workflow for those who want to both submit and await the execution of a transaction in one go.

Sorry @nedsalk I don't follow, can you help me by giving more context here?

My initial thought was to modify the Provider.sendTransaction to use submitAndAwait as I was under the impression that the process was not going to stop and wait till the transaction was processed as submitAndAwait is also a subscription, just like the one being used in this PR statusChange.

Maybe I am missing something here.

UPDATE:

And maybe make the TransactionResponse accept the subscription object on the constructor or another method. Then, there we would call for await.... But yeah, I am aware that this would involve some thinking and maybe it is not the best approach.

I've asked this to bring the submitAndAwait to the conversation.

@nedsalk nedsalk merged commit 8021b38 into rc/salamander Dec 20, 2023
9 checks passed
@nedsalk nedsalk deleted the ns/feat/subscriptions branch December 20, 2023 09:23
arboleya added a commit that referenced this pull request Jan 26, 2024
* chore: updating code owners (#1496)
* docs: purge hardcoded snippets on 'using typegen' page (#1403)
* chore: remove method that's same as base method (#1445)
* chore: implement RC workflow (#1497)
* Revert "feat: add `Predicate.getTransferTxId` helper (#1467)"
* chore: fix rc release string replace (#1529)
* docs: Update some hyperlinks to reference the new documentation hub (#1520)
* chore: improve rc release message (#1559)
* feat: GraphQL subscriptions (#1374)
* chore: pin `graphql-request` to `v5` (#1567)
* chore: upgrade `tsx` (#1574)
* feat: migrate from Jest to Vitest (#1310)
* chore: fix temp test workflow (#1579)
* chore: update required node engine in `create-fuels` (#1582)
* chore: add node version test matrix (#1575)
* chore: fix broken rc message (#1580)
* chore: update nodejs to v20 (#1544)
* feat: accepting addresses as `string` (#1583)
* chore: properly format the PR coverage report comment (#1586)
* fix: flaky test (#1590)
* docs: update `deposit-and-withdraw` page (#1591)
* feat: retry mechanism (#1474)
* feat: replaced `semver` dependency with custom implementation (#1594)
* feat: replace `elliptic` with `@noble/curves` (#1601)
* chore: fix CI failing due to missing tag in test (#1614)
* feat: improve ABI Coders `decode` validation (#1426)
* fix: do not generate a coverage diff without coverage artifact (#1629)
* chore: pinpoint vitest to 1.0.4 (#1637)
* chore: remove `ethers` dependency from `utils` (#1640)
* fix: `getOperation` for `Transfer Asset` (#1619)
* fix: remove external font dependencies (#1642)
* fix: generate RC PR comment on `pull_request` event only (#1648)
* fix: fix failing `rc` comment (#1657)
* chore: add missing test group (#1658)
* feat: implement browser compatibility testing (#1630)
* chore: fix string replace in `rc` ci (#1659)
* chore: adding extra reporters (#1661)
* chore: manually trigger `rc` CI (#1660)
* feat: use `submitAndAwait` graphql endpoint (#1615)
* fix: flaky retry test (#1654)
* feat: create a wallet without a provider (#1566)
* chore!: Share single chainConfig and review node-related utilities (#1602)
* chore: use new temporary coverage artifact (#1676)
* fix: internalizing `findBinPath` utility (#1679)
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
6 participants