Skip to content

refactor: consolidate subscription into execution #3195

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 11 commits into from
Closed

refactor: consolidate subscription into execution #3195

wants to merge 11 commits into from

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented Jun 22, 2021 •
edited
Loading

subscription operations are described by the 'Execution' section of the spec.

@yaacovCR yaacovCR mentioned this pull request Jun 22, 2021
Closed
@yaacovCR yaacovCR requested a review from IvanGoncharov June 22, 2021 12:24
@yaacovCR yaacovCR mentioned this pull request Jun 22, 2021
Closed
@yaacovCR yaacovCR force-pushed the aggregate-error branch 2 times, most recently from e4f2ace to d8e3254 Compare October 3, 2021 20:44
@yaacovCR yaacovCR force-pushed the consolidate-subscription branch 4 times, most recently from dc88bcf to 112feed Compare October 5, 2021 18:05
@yaacovCR yaacovCR changed the base branch from aggregate-error to main October 5, 2021 18:07
@yaacovCR yaacovCR force-pushed the consolidate-subscription branch from 112feed to 83a7f41 Compare October 5, 2021 18:10
@yaacovCR
Copy link
Contributor Author

yaacovCR commented Oct 5, 2021

@IvanGoncharov I have lifted the dependency for this PR on the #3192

This is now just code reorganization/polish, and I believe can be released in v16.

IvanGoncharov
IvanGoncharov requested changes Oct 6, 2021
View reviewed changes
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.

Can you please make a separate PR where you just merge the subscription folder to execute.
That way we don't lose history on subscribe.ts and also it would be way easier to see changes in that file.

@IvanGoncharov
Copy link
Member

I believe can be released in v16.

One issue though is that people using graphql/subscription in imports and configs.
Can you please keep src/subscription/index.ts where you reexport necessary functions from src/execute/index.ts with a note that it's deprecated and will be removed in v17?

IvanGoncharov
IvanGoncharov reviewed Oct 6, 2021
View reviewed changes
src/execution/__tests__/subscribe-test.ts Outdated
expect(await createSourceEventStream(schema, document)).to.deep.equal(
result,
);
const exeContext = buildExecutionContext(
Copy link
Member

Choose a reason for hiding this comment

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

buildExecutionContext is not a part of public API so shouldn't use it in our tests
createSourceEventStream is part of public API so it should be usable as previously with schema + document.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

@yaacovCR
Copy link
Contributor Author

yaacovCR commented Oct 6, 2021

Can you please make a separate PR where you just merge the subscription folder to execute. That way we don't lose history on subscribe.ts and also it would be way easier to see changes in that file.

I have added some more commits where I removed the subscribe.ts file entirely (it's superfluous now that subscribe is part of execution. So I am not sure how we are going to avoid losing the hx from that file. Thoughts?

@yaacovCR yaacovCR mentioned this pull request Oct 7, 2021
Merged
@yaacovCR
Copy link
Contributor Author

yaacovCR commented Oct 7, 2021

Can you please make a separate PR where you just merge the subscription folder to execute. That way we don't lose history on subscribe.ts and also it would be way easier to see changes in that file.

see #3292

I will work soon on rebasing this PR and addressing the other code review, but I suppose we can pick up at least from there.

yaacovCR added 10 commits October 7, 2021 17:47
@yaacovCR
refactor: introduce handleRawError
428efec
@yaacovCR
refactor: introduce executeQueryOrMutation
46cc6dd 
...and executeQueryOrMutationRootFields

to prepare for integration of executeSubscription
@yaacovCR
remove buildExecutionContext from try block
59228a3 
this function currently does not throw GraphQLErrors -- it is not within the parallel try block within execute
@yaacovCR
refactor: use handleRawError with subscriptions
a15298e
@yaacovCR
refactor: rename executeSubscription
f1a5adc 
to executeSubscriptionRootField
@yaacovCR
refactor: move underlying subscription methods
b01cbc7 
...to execution file
@yaacovCR
refactor: executeSubscriptionRootField
092ef63 
to operate only on ExecutionContext
@yaacovCR
introduce executeSubscription
91f18f4
@yaacovCR
refactor: streamline exeContext access
02f2100 
-- In at least a few cases, destructuring assignment from exeContext can improve code readability.
-- Overrides to exeContext can be set using object spread syntax.
@yaacovCR
refactor: consolidate more logic within buildExecutionContext
781e1c1 
Execution arguments object can be passes as-is to the function.
assertValidExecutionArguments can be called within the function rather
than prior
@IvanGoncharov
Copy link
Member

IvanGoncharov commented Oct 7, 2021 •
edited
Loading

I have added some more commits where I removed the subscribe.ts file entirely (it's superfluous now that subscribe is part of execution. So I am not sure how we are going to avoid losing the hx from that file. Thoughts?

@yaacovCR It totally ok to lose history. I just prefer to have as small as possible bridge PR for that, we should separate commit where we move content and also keep content intact as much as possible.
That way in git blame it's clear that code was the move from another file without changes so it's just a matter of switching to parent commit and git blame on the removed file.

@yaacovCR
introduce executeRequest function
758d31e 
...capable of processing requests of all operation types.

ExecutionArgs now augmented with all relevant arguments for
subscriptions as well.
@yaacovCR yaacovCR force-pushed the consolidate-subscription branch from 535e210 to 758d31e Compare October 7, 2021 14:51
@yaacovCR
Copy link
Contributor Author

yaacovCR commented Oct 7, 2021

ok the separate PR for code move is done, and this PR is rebased, but I still need to address your other feedback

@yaacovCR
Copy link
Contributor Author

yaacovCR commented Oct 7, 2021

actually i think the rebase was a bit unsuccessful, will go back through it commit by commit later. maybe i will find something else to break out into a different PR as well

@IvanGoncharov
Copy link
Member

@yaacovCR sounds good 👍
BTW. I find this plugin to be super useful for following file history on GitHub https://chrome.google.com/webstore/detail/follow-for-github/agalokjhnhheienloigiaoohgmjdpned

@yaacovCR
Copy link
Contributor Author

yaacovCR commented Oct 7, 2021 •
edited
Loading

Closed this in favor of PR stack 3293 => 3302.

@yaacovCR yaacovCR closed this Oct 7, 2021
@yaacovCR yaacovCR deleted the consolidate-subscription branch October 7, 2021 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IvanGoncharov IvanGoncharov IvanGoncharov requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yaacovCR @IvanGoncharov