Execute iterator exploration #1
Conversation
dpchamps
force-pushed
the
execute-iterator-exploration
branch
from
1df03f1
to
1fa76be
Compare
|
|
||
| return args; | ||
| } | ||
| const nextIterValue = (typedIter: TypedIterator) => |
There was a problem hiding this comment.
Typing this is quite challenging. We need to indicate that the result of the iterator is either one type or the other so that we can map the actual value and decorate the result with some data needed to "schedule" them.
| ]); | ||
|
|
||
| while (iteratorMap.size > 0) { | ||
| const result = await Promise.race(iteratorMap.values()); |
There was a problem hiding this comment.
Greedily take the next ready value.
Consume each individual iterator lazily: Only tick the last iterator to have won the race.
| runProcess(event, options); | ||
| } | ||
| if (result.type === 'build' && result.value.success) { | ||
| await subProcessRunner.handleBuildEvent(result.value); |
There was a problem hiding this comment.
Guarantee that a build event resets the process runner with the latest build.
| return deferred; | ||
| }; | ||
|
|
||
| export class PromiseQueue<T> { |
There was a problem hiding this comment.
This data structure is necessary to take the push-based child process into a pull-based async iterator
| @@ -97,3 +98,15 @@ export interface NormalizedBuildNodeBuilderOptions | |||
| extends BuildNodeBuilderOptions { | |||
| webpackConfig: string[]; | |||
| } | |||
|
|
|||
| export interface NodeExecuteBuilderOptions { | |||
There was a problem hiding this comment.
I moved this type into utils, because SubProcessRunner uses it as well.
| (devkit.runExecutor as any).mockImplementation(function* () { | ||
| yield { success: true, outfile: 'outfile.js' }; | ||
| yield { success: true, outfile: 'outfile.js' }; | ||
| }); |
There was a problem hiding this comment.
The old test cases for this spec and the one below were non-deterministic:
The state of subProcess was managed as a file-level global previously. These tests would only pass when at least one previous test had been run to populate the subProcess var.
With that subProcess state encapsulated, there was no way to make this test pass without altering it.
| @@ -83,6 +88,7 @@ describe('NodeExecuteBuilder', () => { | |||
|
|
|||
| it('should build the application and start the built file', async () => { | |||
| for await (const event of executeExecutor(testOptions, context)) { | |||
| if (!('success' in event)) throw new Error('Got Unexpected event'); | |||
There was a problem hiding this comment.
Represents a breaking change: the type of event yielded by executeExecutor is now https://github.com/dpchamps/nx/pull/1/files#diff-8b1daad050dbc745646b89f91cd20c497a23f51caf7eeaddb96a517a80b8f01fR28
| error: unknown; | ||
| }; | ||
|
|
||
| export class SubProcessRunner { |
There was a problem hiding this comment.
Happy to write tests for this, awaiting feedback for where this should be in the project structure. It seems like it belongs colocated to executor.impl.ts, but I didn't see other examples of doing stuff like that.
The executor.impl tests cover the integration of this in full, but it felt big enough to split into a separate file. Let me know what you think!
Current Behavior
The node:execute builder immediately exits after build when --watch=false.
Expected Behavior
The node:execute builder wait for the subprocess to exit, or SIGTERM to be received, before exiting. This is how things worked previously.
Related Issue(s)
Notes
The solution here is to essentially "cooperatively schedule" (using the term loosely) events emitted from the
startBuildgenerator, and the events emitted from the child process.We must allow build events to propagate (for example, when
watchmode is configured), while also ensuring that the executor waits for the current process to run to completion.I've implemented some things as classes here, because my sense was this was more in the style of the repo. Happy to capture them differently, if the team would like. Have dropped some github comments along the way, happy to discuss the impl furthur!