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

refactor: execution methods into Executor class #3185

Merged
merged 20 commits into from Jun 21, 2021

Conversation

yaacovCR
Copy link
Contributor

see: #3163 (comment)

depends on #3184

@yaacovCR
Copy link
Contributor Author

Hi @IvanGoncharov

I see your point, that is functionally equivalent, and it does seem cleaner.

So....I think I have the wrapping execute and subscribe functions as slim as they can go, and I have combined the Executor and SubscriberExecutor classes into one.

This is ready again for review. Please feel free to edit/rename/reformat within this PR as necessary.

I am feeling very good about this! I think this will unlock a lot of potential for experimentation from which the ecosystem will benefit greatly. I'd love to see this within the next alpha.

@yaacovCR
Copy link
Contributor Author

In particular, I don't love how I left the organization of folders/directories. The breakdown in structure between execution and subscription does not seem to make sense anymore, as all the essential logic anyway is within the Executor class in the executor.ts. One thought would be to move all the subscription files into the execution folder also -- but it is somewhat helpful having more folders to help keep the tests organized. Back on the other hand, the subscription tests are really just testing the Executor, which doesn't live in that folder. I am going to push one more commit that just moves everything to execution, but feel free to change this PR at will.

@yaacovCR yaacovCR changed the base branch from main to refactor-execution June 21, 2021 21:40
@yaacovCR yaacovCR merged commit beaca66 into graphql:refactor-execution Jun 21, 2021
@yaacovCR yaacovCR deleted the refactor-executor branch June 21, 2021 21:48
@yaacovCR
Copy link
Contributor Author

merging this into branch on main repo, will split out into separate PRs as possible

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

Successfully merging this pull request may close these issues.

None yet

1 participant