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

Pino v7 shows "Cannot find module 'lib/worker.js' when bundling it with Rollup #1157

Closed
pan93412 opened this issue Oct 14, 2021 · 12 comments
Closed

Comments

@pan93412
Copy link
Contributor

pan93412 commented Oct 14, 2021

Brief Description

Rollup can't bundle the worker.js of Pino v7 properly. There may be a Rollup plugin for bundling worker.js, but Pino seems like not compatible with this.

➜  UnblockNeteaseMusic git:(feat/pino-7) ✗ node app.js

node:internal/event_target:777
  process.nextTick(() => { throw err; });
                           ^
Error: Cannot find module '/Users/pan93412/Projects/UnblockNeteaseMusic/precompiled/lib/worker.js'
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:933:15)
    at Function.Module._load (node:internal/modules/cjs/loader:778:27)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:79:12)
    at MessagePort.<anonymous> (node:internal/main/worker_thread:187:24)
    at MessagePort.[nodejs.internal.kHybridDispatch] (node:internal/event_target:562:20)
    at MessagePort.exports.emitMessage (node:internal/per_context/messageport:23:28)
Emitted 'error' event on Worker instance at:
    at Worker.[kOnErrorMessage] (node:internal/worker:289:10)
    at Worker.[kOnMessage] (node:internal/worker:300:37)
    at MessagePort.<anonymous> (node:internal/worker:201:57)
    at MessagePort.[nodejs.internal.kHybridDispatch] (node:internal/event_target:562:20)
    at MessagePort.exports.emitMessage (node:internal/per_context/messageport:23:28) {
  code: 'MODULE_NOT_FOUND',
  requireStack: []
}

Reproducible code and step

https://github.com/UnblockNeteaseMusic/server/blob/316766433de074ff94f120f515e058de63dae560/src/logger.js

git clone https://github.com/UnblockNeteaseMusic/server
cd server
git checkout 316766433de074ff94f120f515e058de63dae560
yarn
yarn build
node precompiled/app.js # ⚠️
@mcollina
Copy link
Member

Thanks for the docs fix!

@pan93412
Copy link
Contributor Author

pan93412 commented Oct 15, 2021

Wait. It is not related to that fix!

@mcollina mcollina reopened this Oct 15, 2021
@mcollina
Copy link
Member

Ah! Sorry about this, I closed the wrong one.

I'm not sure how bundling would work for this as we need to point to a separate file that will be loaded from disk by node, definitely from outside the bundle.

@mcollina
Copy link
Member

Ref: #1104

@zekth
Copy link
Contributor

zekth commented Oct 15, 2021

I'm not sure how bundling would work for this as we need to point to a separate file that will be loaded from disk by node, definitely from outside the bundle.

This would need a specific import to be injected by the bundler. Something like:

import worker from 'web-worker:./<path>/worker.js';

And in the rollup conf:

import webWorkerLoader from 'rollup-plugin-web-worker-loader';
...
export default [
{
  ....
  plugins: {
    webWorkerloader({targetPlatform: 'browser', inline: true}),
  }
]

At the first glance it would chance the pino API to inject the reference right?

@mcollina
Copy link
Member

likely so.

Here is where we instatiate the actual worker object: https://github.com/pinojs/thread-stream/blob/79333e70994fb999300d0fcd73a3db12d69b8710/index.js#L39.

Note that we should not use webworkers but worker_threads and target node.

@zekth
Copy link
Contributor

zekth commented Oct 15, 2021

Is the idea of injecting the bundled worker within the pino.transport options is thinkable? Like:

// application code
pino.transport({
  bundler: true
  ....
})
// pino
const w = require('lib/worker.js')
createWorker(stream,{ worker:w, evaluate:true}
// thread-stream
function createWorker (stream, opts) {
    const { filename, workerData, evaluate } = opts
    let worker
    if (evaluate === true) {
      worker = new Worker (opts.worker.toString(),{
      ...opts.workerOpts,
      eval:true,
      workerData: {
        dataBuf: stream._dataBuf,
        stateBuf: stream._stateBuf,
        workerData
      }
    })
    } else {
      const toExecute = join(__dirname, 'lib', 'worker.js')
      worker = new Worker(toExecute, {
      ...opts.workerOpts,
      workerData: {
        filename: filename.indexOf('file://') === 0
          ? filename
          : pathToFileURL(filename).href,
        dataBuf: stream._dataBuf,
        stateBuf: stream._stateBuf,
        workerData
      }
    })
  }
}

Then the ThreadStream contructor would rely on filename or injected string of the Worker.
Drawback of this, it adds overlap of conditionnal requiring of files.

@mcollina
Copy link
Member

Note that the entrypoint is https://github.com/pinojs/thread-stream/blob/main/lib/worker.js, so we can already make it evaluate.

@Everspace
Copy link

This is also a problem with esbuild for console applications.

@wd-David
Copy link
Contributor

@Everspace can you give esbuild-plugin-pino a try?
I just wrote it to solve my work project. Like to know if it misses any special case.

@Everspace
Copy link

@davipon sorry for getting back to you so late! By the time this happened I had already moved away from pino on the project I was spinning up, and haven't had the need to bundle a logger in an app since.

I will let you know when I do however!

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants