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

Fix the workers path calculations #161

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

segevfiner
Copy link
Contributor

  1. Remove pinoBundlerRan check in case there are multiple pino.js modules around, might fix some cases of the __bundlerPathsOverrides being missing leading to it looking for lib/worker.js and failing
  2. Use a simple __dirname based strategy for pinoBundlerAbsolutePath, more complex outputs layout might require some more thought on how to figure out the relative path from the current file to the workers if they are not together - fixes Using process.cwd is problematic #159
  3. Move pinoBundlerAbsolutePath into a virtual module so it can safely use import.meta.url or __dirname depending on the format. This makes it work in ESM mode, minus the "Dynamic require is not supported" issue of esbuild, which needs a workaround in your esbuild config - How to fix "Dynamic require of "os" is not supported" evanw/esbuild#1921

@wd-David
Copy link
Owner

wd-David commented Jun 1, 2024

Thank you for the PR.
I have limited time to investigate the solution, so it would be great if you could update the test files according to the change.

@segevfiner segevfiner marked this pull request as draft June 2, 2024 07:47
@segevfiner
Copy link
Contributor Author

segevfiner commented Jun 2, 2024

more complex outputs layout might require some more thought on how to figure out the relative path from the current file to the workers if they are not together - fixes #159

They are failing due to having a test that checks such a scenario as I wrote in the PR description, and I can't figure out a way to handle it.

Essentially we are pushing our patch into the pino.js module which gets loaded just once even for multiple entry points, __dirname resolves at runtime to the directory containing this entry point, and we have no obvious way to know where the rest of the entry points are, that is, the additional worker entry points we added. esbuild doesn't seem to handle require.resolve and similar to entry points ATM. I'm not really sure if esbuild has any mechanism to get the correct path to another entry point ATM when they are spread out like this.

The problem is that using process.cwd or absolute paths, as currently used is not helpful, as it makes the bundle either not relocatable, or dependent on the working directory, which is not a reliable way to load your files as that can be anything for a program a user is going to launch via CLI for example. So this just kinda punts the problem into the user invoking to set the working directory to where those worker files are...

I guess one workaround is to make pinoBundlerAbsolutePath be a find-up like search... Though we will need to depend on that module or implement our own version of it.

Unless esbuild has some way to correctly inject the relative path to another entry point @evanw?

@wd-David wd-David marked this pull request as ready for review June 9, 2024 04:14
@segevfiner segevfiner force-pushed the fix-worker-paths branch 3 times, most recently from aa96d3d to 7ca61b3 Compare September 16, 2024 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using process.cwd is problematic
2 participants