-
Notifications
You must be signed in to change notification settings - Fork 643
refactor plugin + worker interface #19
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
Conversation
e6ad72e to
6356a42
Compare
6356a42 to
a30a344
Compare
rekmarks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great start!
I have a couple of significant change requests that I left and explain inline.
In addition to those, I think it's worth considering how the current execution environment (EE) service interface will work when we have multiple EEs. Ideally, there should be a single EE service class that's capable of starting a plugin in any conceivable EE. In that paradigm, the worker EE service would be a sub-service of that uber-EE service. As I mention inline, I believe that some of the existing public methods of the service interface are too low-level for the plugin controller, but they would be completely appropriate for a class that manages multiple EE services.
packages/controllers/src/services/WorkerExecutionEnvironment.ts
Outdated
Show resolved
Hide resolved
packages/controllers/src/services/WorkerExecutionEnvironment.ts
Outdated
Show resolved
Hide resolved
packages/controllers/src/services/WorkerExecutionEnvironment.ts
Outdated
Show resolved
Hide resolved
packages/controllers/src/services/WorkerExecutionEnvironment.test.ts
Outdated
Show resolved
Hide resolved
packages/controllers/src/services/PluginExecutionEnvironmentService.ts
Outdated
Show resolved
Hide resolved
…to WebWorkerExecutionEnvironmentService
7517de2 to
6deb30f
Compare
6deb30f to
963c8bb
Compare
963c8bb to
c996ee4
Compare
rekmarks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just a couple of things to iron out.
packages/controllers/src/services/WebWorkerExecutionEnvironmentService.ts
Outdated
Show resolved
Hide resolved
packages/controllers/src/services/WebWorkerExecutionEnvironmentService.ts
Outdated
Show resolved
Hide resolved
packages/controllers/src/services/ExecutionEnvironmentService.ts
Outdated
Show resolved
Hide resolved
packages/controllers/src/services/WebWorkerExecutionEnvironmentService.ts
Show resolved
Hide resolved
…tService.ts Co-authored-by: Erik Marks <25517051+rekmarks@users.noreply.github.com>
rekmarks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! There are a couple of pre-existing issues I noticed during review that can be addressed during a follow-up.
this changes the interface names to not have
workerin them, also changes the interface to them as to not requireworkerId, but use thepluginNameinstead.It establishes a
ExecutionEnvironmentServiceinterface for other Execution Environment Services to follow and used in thePluginControllerto be able to swap out execution environments.It also renames
WorkerControllertoWebWorkerExecutionEnvironmentServicewhich implementsExecutionEnvironmentService.