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

JS-400 Single entrypoint for worker and server #4893

Merged
merged 6 commits into from
Nov 8, 2024
Merged

Conversation

vdiez
Copy link
Contributor

@vdiez vdiez commented Nov 5, 2024

@vdiez vdiez requested a review from zglicz November 5, 2024 15:43
@hashicorp-vault-sonar-prod hashicorp-vault-sonar-prod bot changed the title Single entrypoint for worker and server JS-400 Single entrypoint for worker and server Nov 5, 2024
@vdiez vdiez force-pushed the single-entrypoint branch from 2330dad to 7f6a405 Compare November 5, 2024 15:52
@vdiez vdiez force-pushed the single-entrypoint branch from 90c229f to ff81757 Compare November 6, 2024 21:52
@vdiez vdiez enabled auto-merge (squash) November 8, 2024 09:58
@ericmorand-sonarsource
Copy link
Contributor

I find the pattern strange, and more generally, I don't see why we need a different script for the server and the worker.

I would have gone with this pattern:

#!/usr/bin/env node
import { Worker, isMainThread, parentPort }  from "node:worker_threads";

if (isMainThread) {
  // this is the parent, aka the CLI script
  const worker = new Worker(import.meta.filename);

  worker.once("message", (message) => {
    console.log('The worker did its thing', message);
  });

  worker.postMessage('Do something');
}
else {
  // this is the worker
  console.log('ParentPort is guaranteed to exist since we are not in the main thread');

  parentPort.once("message", (message) => {
    if (message === 'Do something') {
      parentPort.postMessage("I did what I am supposed to do");
    }
  });
}

There seems to be some partial implementation of this pattern - the isMainThread in server.mjs and the if (parentPort) in worker.ts - which adds to the confusion.

Copy link
Contributor

@zglicz zglicz left a comment

Choose a reason for hiding this comment

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

lgtm


/**
* Handler to analyze in the same thread as HTTP server. Used for testing purposes
* @param type
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comment @param type helping?

Comment on lines +70 to +76
case 'success':
if (typeof message.result === 'object' && outputContainsAst(message.result)) {
sendFormData(message.result, response);
} else {
response.send(message.result);
}
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

no curly braces around multiple statements? or is an if simple enough?

@vdiez vdiez merged commit 0e9590e into master Nov 8, 2024
17 of 18 checks passed
@vdiez vdiez deleted the single-entrypoint branch November 8, 2024 11:27
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.

3 participants