fix loading .js with pkg

[burgerni10]

Fix #1237 : With pkg, .js files are not loaded correctly. This fix comes with a PR on thread-stream : pinojs/thread-stream#95.

After a look at previous work by @rokku-x to fix this issue :
#1254 (pino)
pinojs/thread-stream#53 (thread-stream)
pinojs/real-require#13 (real-require)

I think there is no need to change real-require and just load with realRequire .js files. These fix with the latest version of the pino lib seem to work with pkg bundler, but I'm not sure how it may impact others bundler.

[mcollina]

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

[burgerni10]

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

I have some difficulties of accessing this code from the tests file. Can someone help me or give me some hints on how to do so please ? (I have the same problem for the thread-stream PR - pinojs/thread-stream#95 - related to this one)

[mcollina]

I have no clue on how to test it :(.

[burgerni10]

I'll try to find a way, but I may take some time. If someone passing by can give me a hand, please do so :)

[rokku-x]

I am sorry I was long away from the project where I used this. As far as I remember the problem was related to the "new Function()" used by realImport. It makes the compiled pkg executable throw error at runtime. I remember I also edited the real-require package as the realImport variable Initialization will throw the error. I do not know If I remember correctly but this is what I remember

[burgerni10]

@mcollina, after some more tries, I didn't manage to test the code running inside a worker thread (neither with tap nor jest). From what I understand with the current tests, both transport-stream.js in this repo and worker.js in the thread-stream repo are not tested.
Would it be ok if I add some comments explaining the issue being fixed and that it is not tested in the CI ?

[mcollina]

They are tested but the coverage report system cannot detect them.

[mcollina]

The reason why a test is needed it's because I don't want to claim this use case is supported without actual proof this is supported.

[burgerni10]

Thank you for your answer ! I understand your point and I totally agree with it.

Using breakpoints, I noticed that the existing test in bundlers.support.test.js already took care of testing my fix. However, the tests were only testing .js file and no realImport were called, so I've just added another test with to-file-transport.es2017.cjs

Let me know if I can do more. I'll try to test the thread-stream fix.

[mcollina]

lgtm

[burgerni10]

Hi @rokku-x,

Yes it is linked to the realImport. But your fix was checking if 'process' exists (which is always the case since it is used in NodeJs) and use a realRequire if it exists.

Now this fix tries realImport, and if an error is caught, tries again with realRequire. I tested it with pkg on my projec: both fix from thread-stream and this one make it works. Can you try my modifications on your use case ?

[rokku-x]

@burgerni10 if that is the case, it will end up the same fix I did when I used it in my last project. The fix by adding try catch to pino and realrequire is not enough as far as I remember. The error is being throwed at the process context because the error happens at the initialization time of the function variable and the module.

I will try this on weekend.

[burgerni10]

@rokku-x Any updates of the test ? mcollina has merged a similar PR on thread-stream, so if you update thread-stream to the latest version, and this PR in your project with pkg, it should be enough to test it.

[mcollina]

This looks ok to me, I'll land and release.

[github-actions]

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