dop251 / goja_nodejs

Nodejs compatibility library for Goja
MIT License
337 stars 81 forks source link

require: Add partial implementation of module search algorithm #18

Closed nwidger closed 4 years ago

nwidger commented 4 years ago

Add a partial implementation of the Node.js module search algorithm described here:

https://nodejs.org/api/modules.html#modules_all_together

The functions LOAD_AS_FILE, LOAD_INDEX, LOAD_AS_DIRECTORY, LOAD_NODE_MODULES and NODE_MODULES_PATHS outlined in the pseudocode linked above are implemented, whereas the functionality outlined in LOAD_SELF_REFERENCE and LOAD_PACKAGE_EXPORTS is missing.

The module resolution algorithm is implemented via a new (RequireModule).resolve(path string) method which returns the resolved path to a file that can be loaded using the Registry's SourceLoader. The returned resolved path is always cleaned via filepathClean. The resolve method uses the Registry's SourceLoader via the new (Registry).getSource(path string) method to search for modules and supports resolving both native ("core") and JavaScript modules.

Add new resolveStart string field to RequireModule. resolveStart is used to store the initial start path for the module search algorithm in the resolve method. When require() is called from the global context, resolveStart should be set to the directory of the currently executing script. This information is stored in r.runtime.Program.src.name but is not currently exported, therefore for now resolveStart is set to ".". During a nested require() call, resolveStart is set to the directory of the module currently being loaded.

Add new constructor NewRegistry which takes a variadic argument of Option's. Currently, valid options are WithLoader and WithGlobalFolders.

Add new globalFolders string slice to Registry. globalFolders stores additional folders that are searched by require() and is used in NODE_MODULES_PATHS in the pseudocode linked above. By default, a Registry has an empty globalFolders slice, but this can be changed using WithGlobalFolders.

Add support for loading modules with a .json extension by passing the contents of the file through JavaScript's JSON.parse and assigning the return value to module.exports within the module wrapper function.

Add new test TestResolve to test module search algorithm.

Fixes #5.

nwidger commented 4 years ago

@dop251 Here's my initial attempt at the module search algorithm. I see there're some issues probably related to the use of filepath.Clean on Windows in the Travis CI build. I'll try to figure out what might be causing that, it might be difficult though since I don't have a Window machine to test on.

nwidger commented 4 years ago

@dop251 I believe I fixed the Windows path issue, and fixed a few other minor bugs along the way:

I think this should be ready to look at now. Thanks!

nwidger commented 4 years ago

I just stumbled on this package which is used by the Hugo project and which contains a module resolver:

https://github.com/evanw/esbuild/blob/master/internal/resolver/resolver.go

It appears to be a more full-featured implementation that we could perhaps use as a reference.

dop251 commented 4 years ago

Have a look at https://github.com/dop251/goja_nodejs/commit/8b6014989c479d2c5101bc1216c71ed9fbeb6d66 It will currently only work with es6 branch of goja, because Runtime.CaptureCallStack() is only available there. I intend to merge it into master shortly though. Let me know if it works for you.

nwidger commented 4 years ago

@dop251 I left a few more comments here, I think the infinite loop issue is the only major one. Thanks so much for taking the time to help me with this feature. :)

dop251 commented 4 years ago

All should be fixed now

nwidger commented 4 years ago

@dop251 I think there might still be an issue lurking somewhere. I added a test that tries to load the core-js module and it isn't happy.

cd $GOPATH/src/github.com/dop251/goja_nodejs/require/testdata
npm install core-js

then I added this test

func TestRequireCoreJS(t *testing.T) {
    vm := js.New()
    r := NewRegistry()
    rr := r.Enable(vm)
    _, err := rr.Require("./testdata/node_modules/core-js")
    if err != nil {
        t.Fatal(err)
    }
}

Here's the output, with some extra debug logs that I added:

https://gist.github.com/nwidger/38d5417cfff1375eb321991badf5b13d

dop251 commented 4 years ago

It's a problem with the error propagation: currently all errors are treated as if the file didn't exist so the search continues. I have fixed that which revealed the real error: missing RegExp.prototype.[@@replace] 😦 RegExp is still incomplete in es6

nwidger commented 4 years ago

Damn, I ran this test successfully a few weeks ago using the master branch of goja and the branch for this MR. I guess whatever feature detection core-js uses get tripped up running on the es6 branch of goja. I suppose it's good that it's not a problem with the resolution algorithm, though.

I suppose the way to make this error more obvious would be to have loadModule return two classes or errors, one indicating that the file was found but an error occurred while evaluating it, and another indicating that the file couldn't be found/opened. We could then stop the algorithm in the case that we find the file but it just doesn't run successfully. Is that the basic idea of what you were thinking of?

EDIT: Woops I completely missed your commit, nevermind!

nwidger commented 4 years ago

Everything looks good to me at this point, is there anything else you're still thinking about tweaking? If not, are you thinking you'll hold off on merging this until es6 is merged into goja's master? I suppose either we just wait to merge, we have getCurrentModulePath fall back to returning . if the runtime doesn't have a CaptureCallStack method or we backport CaptureCallStack into goja master (no idea how much work that would be). I don't mind either way, and I'm not trying to rush you, I'm just curious.

dop251 commented 4 years ago

I'd like to finish off RegExp, then merge es6. It's taking a bit longer than I was hoping, sorry about that. Good news is it's almost done now.

nwidger commented 4 years ago

Sounds good, I'll be cheering you on from the sidelines. :)