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 @ninjutsu-build/node dyndep for CommonJS #38

Closed
elliotgoodrich opened this issue Mar 25, 2024 · 2 comments · Fixed by #73
Closed

Fix @ninjutsu-build/node dyndep for CommonJS #38

elliotgoodrich opened this issue Mar 25, 2024 · 2 comments · Fixed by #73
Labels
bug Something isn't working

Comments

@elliotgoodrich
Copy link
Owner

I don't think the require hook is correctly working in the node plugin as dyndeps for CommonJS entry points are not working.

assert.deepEqual(getDeps(), {
[output]: [script, one, two],
[output2]: [script2], // FIX: Should also depend on `two`
});

The import hook runs in a different thread to the rest of the code and this is where we open the file. I assume that the require hook code is failing to see that a file is open because the handle is null in the main thread.

Perhaps passing over a MessagePort would be the solution and to keep IO on the main thread.

@elliotgoodrich elliotgoodrich added the bug Something isn't working label Mar 25, 2024
@elliotgoodrich
Copy link
Owner Author

Using MessagePort seems to hang unless we explicitly call port2.close() #69

However, we don't know when to call port2.close() so I don't think we can use this method. This was copying the example in the node docs so I think there may be a bug somewhere.

@elliotgoodrich elliotgoodrich linked a pull request May 8, 2024 that will close this issue
@elliotgoodrich
Copy link
Owner Author

Thanks to @dygabo in nodejs/node#52846 we can use the MessagePort, we just need to unref() it first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant