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 loading .js with pkg #95

Merged
merged 3 commits into from
Aug 4, 2022
Merged

fix loading .js with pkg #95

merged 3 commits into from
Aug 4, 2022

Conversation

burgerni10
Copy link
Contributor

Fix pinojs/pino#1237 : With pkg, .js file are not loaded correctly.

See pinojs/pino#1493 for more info

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina
Copy link
Member

Could you add a test to validate we don't regress this?

@burgerni10
Copy link
Contributor Author

I've added a test to load to-file.mjs file in a bundler context, since my fix was already tested with the existing test.

lib/worker.js Outdated
@@ -29,6 +29,9 @@ async function start () {
// TODO: Support ES imports once tsc, tap & ts-node provide better compatibility guarantees.
// Remove extra forwardslash on Windows
fn = realRequire(decodeURIComponent(filename.replace(process.platform === 'win32' ? 'file:///' : 'file://', '')))
} else if (filename.endsWith('.js')) {
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, we cannot assume that .js files are always commonjs. Please read https://nodejs.org/api/packages.html#determining-module-system. This will break folks using it with ESM. I think you might want to rely on the try-catch instead.

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've updated my PR after your review. The error thrown when bundled with pkg is this one (error.code is undefined):

node:internal/event_target:912
  process.nextTick(() => { throw err; });
                           ^

TypeError: Invalid host defined options
    at eval (eval at <anonymous> (C:\snapshot\OIBus\node_modules\real-require\src\index.js:4:20), <anonymous>:3:1)
    at start (C:\snapshot\OIBus\node_modules\thread-stream\lib\worker.js:33:19)
    at Object.<anonymous> (C:\snapshot\OIBus\node_modules\thread-stream\lib\worker.js:88:1)
    at Module._compile (pkg/prelude/bootstrap.js:1930:22)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1159:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:77:12)
    at MessagePort.<anonymous> (node:internal/main/worker_thread:191:24)
    at MessagePort.[nodejs.internal.kHybridDispatch] (node:internal/event_target:643:20)
Emitted 'error' event on ThreadStream instance at:
    at destroy (C:\snapshot\OIBus\node_modules\thread-stream\index.js:349:12)
    at Worker.onWorkerMessage (C:\snapshot\OIBus\node_modules\thread-stream\index.js:164:7)
    at Worker.emit (node:events:527:28)
    at MessagePort.<anonymous> (node:internal/worker:232:53)
    at MessagePort.[nodejs.internal.kHybridDispatch] (node:internal/event_target:643:20)
    at MessagePort.exports.emitMessage (node:internal/per_context/messageport:23:28)
    at Worker.[kOnExit] (node:internal/worker:266:5)
    at Worker.<computed>.onexit (node:internal/worker:198:20)

I tried to test it the same way than before and I tried to do so by mocking the worker lib:

  t.mock('../lib/worker.js', {
    'real-require': {
      realImport: () => {throw new Error('oh no')}
      realRequire: () => (modulePath) => {
        if (typeof __non_webpack__require__ === 'function') {
          return __non_webpack__require__(modulePath)
        }

        return require(modulePath)
      }
    }
  })

But with the mock, I lose the access to the worker_threads data. I feel in a deadlock for now.

Can you please let me know if this solution seems acceptable or not before I try again to test this fix ?
The same thing as been done on the pino repo.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit deaedaa into pinojs:main Aug 4, 2022
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.

Pino with transports not compatible with pkg
2 participants