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

chore!: wrap subscriptions in promise #2964

Merged
merged 2 commits into from
Aug 19, 2024

Conversation

nedsalk
Copy link
Contributor

@nedsalk nedsalk commented Aug 16, 2024

Summary

I moved FuelGraphqlSubcriber instantiation to be async as a preparatory step for #2936. I tried introducing the solution for that issue in #2962, commit 8051113, but I wasn't happy with the solution so I reverted it and created this to give myself room to figure out a solution.

This change is necessary for the usage of submitAndAwait endpoint because the transaction can fail when submitted, but the failure isn't manifested until a call to the FuelGraphqlSubscriber.next method is done. With this async instantiation, the subscription can be made to fail before the actual subscription iterator is returned. I will add this failure handling in a subsequent PR that will tackle #2936, after I sync with the fuel-core team. We currently aren't using submitAndAwait anywhere so it's not a problem to leave it unhandled as-is.

Breaking Changes

// before
const subscription = provider.operations.statusChange({ transactionId });
for await (const response of subscription) { ... }

const submittedTxSubscription = this.operations.submitAndAwait({ encodedTransaction: '0x' });
// after
const subscription = await provider.operations.statusChange({ transactionId });
for await (const response of subscription) { ... }

const submittedTxSubscription = await this.operations.submitAndAwait({ encodedTransaction: '0x' });

Checklist

  • All changes are covered by tests (or not applicable)
  • All changes are documented (or not applicable)
  • I reviewed the entire PR myself (preferably, on GH UI)
  • I described all Breaking Changes (or there's none)

Copy link
Contributor

@petertonysmith94 petertonysmith94 left a comment

Choose a reason for hiding this comment

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

May be worth adding submitAndAwait to the breaking changes (to be thorough):

// Before
this.operations.submitAndAwait({ encodedTransaction: '0x' })
// After
await this.operations.submitAndAwait({ encodedTransaction: '0x' })

LGTM though, nice one!

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.

I tried introducing the solution for that issue in #2962, commit 8051113, but I reverted it afterwards because it caused issues.

How this reads: We can't merge #2962 and #2963 together, or we'll have issues.

@nedsalk
Copy link
Contributor Author

nedsalk commented Aug 18, 2024

@arboleya I updated the description.

It's not that we'll have issues by implementing both #2936 and #2962 together but that my solution for #2936 was naive so I reverted it. I actually made it work locally and all the tests were passing but when it came to the CI it started failing because the CI machine is slower and I was doing some sleep(x) tricks which just failed in the CI because the chosen x was not enough. It was generally a bad take so I just reverted it and created this PR in preparation to allow myself more time to figure out the correct solution.

@nedsalk nedsalk requested a review from arboleya August 18, 2024 15:04
Copy link
Contributor

Coverage Report:

Lines Branches Functions Statements
79.18%(-0.01%) 71.45%(+0.02%) 77.31%(+0%) 79.31%(-0.01%)
Changed Files:
Ok File (✨=New File) Lines Branches Functions Statements
🔴 packages/account/src/providers/transaction-request/transaction-request.ts 88.48%
(+0%)
79.71%
(+1.45%)
84%
(+0%)
88.73%
(+0%)

@nedsalk nedsalk merged commit bbd794a into master Aug 19, 2024
20 checks passed
@nedsalk nedsalk deleted the ns/chore/wrap-subscriptions-in-promise branch August 19, 2024 07:59
maschad pushed a commit that referenced this pull request Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Issue is a chore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants