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

Child process should ignore files that are specified in the ignore array #72

Merged
merged 2 commits into from
May 25, 2023

Conversation

jasong689
Copy link
Contributor

@jasong689 jasong689 commented May 18, 2023

Currently the child process will attempt to get the parent process to compile a file even if it's part of your wds.ignored path. This results in "[wds] Internal error: compiled ${filename} but did not get it returned from the leader process in the list of compiled files" being thrown on these files. Checking if the file was ignored in the child process was much too slow and almost tripled the bench results so instead we'll return if the file is ignored from the parent call.

@@ -166,4 +174,6 @@ export const wds = async (options: RunOptions) => {
logShutdown("shutting down project since it's no longer needed...");
project.shutdown(code ?? 1);
});

return server;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Needed to return the server here so that I could close it in my tests

await this.reportErrors(async () => {
await Promise.all(fileNames.map((filename) => this.buildFile(filename, root, swcConfig)));
});
await this.reportErrors(Promise.allSettled(fileNames.map((filename) => this.buildFile(filename, root, swcConfig))));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't entirely sure if this was a bug or not. Files will stop compiling as soon as one file fails with Promise.all, this was fixed so that the benchmarks could be run

@jasong689 jasong689 requested a review from airhorns May 19, 2023 16:55
@jasong689 jasong689 marked this pull request as ready for review May 19, 2023 16:59
Copy link
Contributor

@airhorns airhorns left a comment

Choose a reason for hiding this comment

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

LGTM if gadget still boots fine!

@jasong689 jasong689 merged commit b343562 into main May 25, 2023
2 checks passed
@jasong689 jasong689 deleted the child-process-ignore branch May 25, 2023 14:23
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

2 participants