search

dop251 / goja_nodejs

Nodejs compatibility library for Goja
MIT License
329 stars 78 forks source link

Load the process module as node:process #59

Closed ganigeorgiev closed 1 year ago

ganigeorgiev commented 1 year ago

This is a small PR that load the process module as node:process to prevent conflicts with userland modules (partially related to https://github.com/pocketbase/pocketbase/issues/2292).

dop251 commented 1 year ago

Hi. Looking at the original issue I'm not sure this will help if implemented correctly. According to the nodejs documentation certain built-in modules (which includes process) can be loaded both with and without the node: prefix and always take precedence over user-defined modules on the file system (see here).

The current goja_nodejs implementation follows the second part (the precedence), so the only reason you see the error is because the built-in module is not registered (i.e., not imported in the Go code).

I have now implemented the first part, but before pushing it just wanted to clarify.

ganigeorgiev commented 1 year ago

... so the only reason you see the error is because the built-in module is not registered (i.e., not imported in the Go code).

I'm a little confused. The process module is registered in the goja runtime via something like:

registry := new(require.Registry)

vm := goja.New()
registry.Enable(vm)
process.Enable(vm)

Also note that the process module is not actually used in the goja scripts that are being executed.

My understanding was that because the default goja source loader is used, when internally the process module calls require.Require(runtime, "process") without the prefix it will prioritize whatever dependency is available in the user-defined node_modules directory.

At least internally it fixes the issue for me, but it is possible that the cause could have been something totally else.

Edit: Do you mean that the prefix for this particular case is not needed if the internal require implementation changes? If Yes - I'm OK with not having the prefix.

Edit2: _Looking at the resolve implementation indeed the native modules should be loaded with higher priority, I'm no longer sure why it conflicts with the node_modules dependency. If you don't have the time to further pursue it, I'll try to take a more detailed look at it sometime later this week._

ganigeorgiev commented 1 year ago

Oh, I think I know what was the cause of the issue.

Of course I should have started first from checking what version of goja_nodejs we were using. At the time the issue was reported in my repo, based on my go.mod history, it seems that we were using 3aa5028e57f6. By also looking at the goja_nodejs commits it seems that there was a recent change couple months ago in https://github.com/dop251/goja_nodejs/commit/7ebd95c712e98a03ce191261be80f7de8616dd24 that now prioritizes the native modules, which would explain why with the recent versions this may not be reproducible.

In that case - Yes, the node: prefix doesn't seem to be needed anymore as the process module is registered as native.

I think this PR could be closed but I'll leave it up to you since I'm not fully sure that I understand what do you refer to by "I have now implemented the first part" in your comment.

dop251 commented 1 year ago

I meant this