Fix: in require(), use os.Getwd() when frames[1].SrcName() is empty

[benallfree]

There are cases such as in PocketBase where frames[1].SrcName() is empty.

An empty src name causes getCurrentModulePath() to return ., but that will not traverse up to parent directories to search for ancestor node_modules because parent := path.Dir(start) fails (path.Dir(".") == ".").

This fix returns an absolute path instead of ., which allows resolve() to correctly traverse parents.

Tagging @ganigeorgiev for further discussion and input if needed.

[dop251]

This fix is not correct because resolve() should not assume it's running on a local filesystem (or any filesystem for that matter). Could you please come up with a test case?

[benallfree]

I reverted the change and added a failing test case instead.

Given a package residing in /home/src/node_modules/app16/index.js,

src="" will fail to find it while src="/home/src/packages/a" will work as expected. This is because when getCurrentModulePath() returns ., it will not properly traverse to parent node_modules.

[dop251]

I meant a test case to explain why frames[1].SrcName() is empty.

[benallfree]

Oh, I'm sorry. That is coming from a use case in PocketBase. I believe when PocketBase stringifies JS code so it can be executed inside callback handlers, it loses its SrcName.

I'm not the author of PocketBase so I tagged Gani in case he can offer more insight.

If you feel this is better handled downstream in PocketBase, I will try that instead. I just thought there might be various reasons SrcName might be empty and that there could be a way to handle that case gracefully with Getwd(). But that might be wrong.

[ganigeorgiev]

@benallfree If you think that the issue originate from the fact that in PocketBase we don't specify a "name" when compiling the goja scripts then we can easily generate one, but is this really the issue here?

Could you provide a minimal code sample or repo that illustrates the actual problem (aka. what and how to trigger and see the error you are experiencing)?

[benallfree]

@ganigeorgiev Here is a minimal sample showing that traversal fails in monorepos where packages are hoisted to the monorepo's root node_modules: https://github.com/benallfree/minimal-monorepo-goja-demo

Apologies to you both if I have posted this issue in the wrong place, it appeared at first to be fixable in goja-nodejs but of course a fix in PocketBase would work for my case too.

And Gani, apologies for tagging you in #96 as well. That PR was how I pinpointed the problem and I think it would be very helpful for JSVM users to have module resolution debugging capability instead of just Invalid module with no context. I thought it would be of interest to your project but I will refrain from tagging you in the future.

[ganigeorgiev]

@benallfree I've added a default scripts name in the latest PocketBase master, could you give it a try (see Building and running the repo main.go example)? If it works OK for your use case, I'll push a minor v0.25.5 and backport v0.22.31 releases sometime later today/tomorrow.

Regarding whether this should be fixed in goja_nodejs - the empty script name is usually a result from calling vm.RunString(str), vm.RunScript("", str) and other similar methods. If we assume that vm.RunString(str) should behave the same as eval(str)/node -e str in Node.js then I guess it should be reflected here too but whether that is actually the expectation and the desired behavior - I'm not really sure and I don't have a strong opinion on this matter.

[benallfree]

@ganigeorgiev Yes, it is fixed. Thank you!

@dop251 It does seem that module resolution won't work correctly with vm.RunString(). I'm happy to continue with this PR if you feel it has merit.

[benallfree]

@dop251 I found another issue. The NodeJS module resolution algorithm follows symlinks. pnpm in particular takes advantage of this by hard linking all the packages in ./node_modules/.pnpm and then symlinking to them to build out the rest of the dependency tree.

Example for url-parse:

  // url-prase/package.json

  "dependencies": {
    "querystringify": "^2.1.1",
  },

pnpm will store this as follows:

node_modules/
   .pnpm/
      url-parse@1.5.0/
         node_modules/
             querystringify -> ../../query-stringify@2.2.0/node_modules/querystringify
             url-parse/
                dist/
                package.json
      querystringify@2.2.0/
         node_modules/
            querystringify/
                package.json
   url-parse -> ./.pnpm/url-parse@1.5.0/node_modules/url-parse

Given an index.js such as:

require('url-parse')

Node's resolution will resolve symlinks for the parent path as follows:

./node_modules/url-parse/index.js (ok)
./node_modules/.pnpm/url-parse@1.5.0/node_modules/querystringify/index.js (ok)

Whereas goja_nodejs will do only traverse up the symlink path:

./node_modules/url-parse/index.js (ok)
./node_modules/url-parse/node_modules/querystringify/index.js (not found, evaluates to ./node_modules/.pnpm/url-parse@1.5.0/node_modules/url-parse/node_modules/querystringify/index.js)
./node_modules/querystringify/index.js (not found)

The following code fixes it, but then we are back to module paths may not be filesystem paths:

	fname := frames[1].SrcName()
	println("original", fname)
	if runtime.GOOS != "windows" {
		if resolved, err := filepath.EvalSymlinks(fname); err == nil {
			fname = resolved
		}
	}
	println("resolved", fname)
	return path.Dir(fname)

[dop251]

Please check #101. As it's a rather substantial and potentially breaking change, I'd leave it open for some time to see if there are any objections.