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

introduce new IncrementalPublisher class #3894

Merged
merged 3 commits into from
Jun 17, 2023
Merged

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented May 13, 2023

extracted from #3886

depends on #3903

more refactors from the without-branching branch, a bit more fundamental than #3891

[set as patch release because it does have an observable effect on the number of payloads, see below]

= iterate only through completed items
= remove extra ticks by making the publisher manage changes to its state synchronously.
= use children array instead of promises to manage hierarchy
= have IncrementalPublisher instantiate new IncrementalDataRecords

= The new publisher sometimes cause an empty { hasNext: false } to be emitted. In particular, because the publisher is faster than it was, it may emit a stream result before the stream's asynchronous iterator has completed.
= The new publisher may sometimes reduce the number of { hasNext: false } records that are emitted. For example, when errors on the initial result filter all subsequent results, this now happens synchronously, and so the publisher knows immediately that there are no subsequent results, such that there is no need for an empty final payload.

@yaacovCR yaacovCR requested review from robrichard and a team May 13, 2023 20:00
@netlify
Copy link

netlify bot commented May 13, 2023

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit 177cf6f
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/64884649fc939d0008803330
😎 Deploy Preview https://deploy-preview-3894--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

  • @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
  • @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

@yaacovCR yaacovCR added the PR: bug fix 🐞 requires increase of "patch" version number label May 13, 2023
@yaacovCR yaacovCR changed the title extract Publisher extract Publisher May 14, 2023
@yaacovCR yaacovCR changed the title extract Publisher extract Publisher from current incremental delivery implementation May 14, 2023
@yaacovCR yaacovCR changed the title extract Publisher from current incremental delivery implementation introduce Publisher May 14, 2023
@yaacovCR yaacovCR changed the title introduce Publisher incremental delivery: introduce Publisher May 14, 2023
@yaacovCR yaacovCR force-pushed the extract branch 9 times, most recently from 4296109 to d106636 Compare May 16, 2023 18:18
@yaacovCR yaacovCR force-pushed the extract branch 2 times, most recently from d4baa03 to d55deae Compare May 23, 2023 16:50
@yaacovCR yaacovCR force-pushed the extract branch 5 times, most recently from 1d889d4 to 97d49e2 Compare June 5, 2023 19:28
@yaacovCR yaacovCR force-pushed the extract branch 3 times, most recently from ec1f103 to 202f06b Compare June 6, 2023 13:39
@yaacovCR yaacovCR changed the title incremental delivery: introduce Publisher introduce new IncrementalPublisher class Jun 6, 2023
Copy link
Member

@IvanGoncharov IvanGoncharov left a comment

Choose a reason for hiding this comment

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

Sadly, I'm not in the context of this code :(
So I think @robrichard is the most knowledgeable person on this topic, so I he approves, let's merge this one.

P.S. From a general code quality standpoint, it's good after you add missing private attributes.

src/execution/IncrementalPublisher.ts Outdated Show resolved Hide resolved
src/execution/IncrementalPublisher.ts Outdated Show resolved Hide resolved
= iterate only through completed items
= remove extra ticks by making the publisher manage changes to its state synchronously.
= use children array instead of promises to manage hierarchy
= have IncrementalPublisher instantiate new IncrementalDataRecords

= The new publisher sometimes cause an empty `{ hasNext: false }` to be emitted. In particular, because the publisher is faster than it was, it may emit a stream result before the stream's asynchronous iterator has completed.
= The new publisher may sometimes reduce the number of `{ hasNext: false }` records that are emitted. For example, when errors on the initial result filter all subsequent results, this now happens synchronously, and so the publisher knows immediately that there are no subsequent results, such that there is no need for an empty final payload.
@yaacovCR
Copy link
Contributor Author

yaacovCR commented Jun 10, 2023

P.S. From a general code quality standpoint, it's good after you add missing private attributes.

done! a27ea60

= requires reordering of methods because of eslint rule `@typescript-eslint/member-ordering`
= adds underscore to private `_initialResult` property for consistency
@yaacovCR yaacovCR merged commit d766c8e into graphql:main Jun 17, 2023
@yaacovCR yaacovCR deleted the extract branch June 17, 2023 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: bug fix 🐞 requires increase of "patch" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants