Skip to content
This repository has been archived by the owner on Oct 18, 2023. It is now read-only.

Emits messages on the ipc channel #43

Closed
LinusU opened this issue Oct 26, 2022 · 5 comments
Closed

Emits messages on the ipc channel #43

LinusU opened this issue Oct 26, 2022 · 5 comments

Comments

@LinusU
Copy link

LinusU commented Oct 26, 2022

Hello,

I'm trying to use this loader together with Jest, but are running into some problems. It seems like using this loader causes extra messages to be emitted when spawning a child process using child_process.fork.


Here is a minimal reproduction:

package.json

{
  "type":"module",
  "dependencies": {
    "@esbuild-kit/esm-loader":"^2.5.0"
  }
}

parent.js

import child_process from 'node:child_process'

const child = child_process.fork('./child.js')

child.on('message', (msg) => {
  console.log(msg)
})

child.js

process.send({ hello: 'world' })

Here is the output when running with just regular Node.js

$  node ./parent.js 
{ hello: 'world' }

And here is the output with the loader:

$ node --loader @esbuild-kit/esm-loader ./parent.js
(node:74148) ExperimentalWarning: --experimental-loader is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
(node:74149) ExperimentalWarning: --experimental-loader is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
{ type: 'dependency', path: 'file:///private/tmp/hjdkas/child.js' }
{ hello: 'world' }

Notice the extra { type: 'dependency', path: 'file:///private/tmp/hjdkas/child.js' } line

@privatenumber
Copy link
Member

Related: privatenumber/tsx#131

Ideally, other tools that are using IPC don't assume they're the only ones on it and throw errors on unexpected messages.

I guess I can look into creating a separate socket/server to communicate though.

@LinusU
Copy link
Author

LinusU commented Oct 31, 2022

I've submitted a PR to fix this in Jest: jestjs/jest#13543

If this is indeed only used for watch mode, maybe a solution here could also be that it checks for process.env.ESBUILD_KIT_ESM_LOADER_WATCH or something similar and then only do process.emit if that is present...

@jeysal
Copy link

jeysal commented Nov 8, 2022

Hi, coming from the Jest issue.

As is, Jest does not namespace worker communication in any form and thus it would be dangerous to be lenient and allow other messages. If other code uses IPC with some overlap in protocol, invalid messages from it would no longer cause a fail fast crash and the few messages that are valid could then be misinterpreted without noticing and lead to obscure bugs. I'm not aware of a widely used standard for namespacing IPC safely between different tools/libraries. Given that lack of namespacing, there should not be multiple users of the IPC channel, and I think it's reasonable for the process that spawns the child (such as jest-worker) to get control of the IPC.

However, if there is any sort of standard namespacing for node IPC that you could point me to, it could make sense for Jest to follow that and leave the rest of the channel up to other consumers. Those could then of course still conflict if they are themselves not namespacing properly, but that wouldn't be jest-workers problem.

@npdev453
Copy link
Contributor

npdev453 commented Aug 6, 2023

Seems like can be closed, because jestjs/jest#13543 was merged

@privatenumber
Copy link
Member

Great, thanks!

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

4 participants