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

require: Add partial implementation of module search algorithm #18

Merged
merged 1 commit into from
Aug 11, 2020

Conversation

nwidger
Copy link

@nwidger nwidger commented Jul 14, 2020

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
Copy link
Author

nwidger commented Jul 14, 2020

@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 nwidger force-pushed the module-search-algorithm branch 3 times, most recently from fdf3794 to ae6e33a Compare July 15, 2020 15:22
@nwidger
Copy link
Author

nwidger commented Jul 15, 2020

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

  • (*RequireModule).resolveAsFile now only skips appending a .js or .json extension if the path already has one of those two extensions. Previously, a path such as es.symbol would also be skipped since filepath.Ext would report that it had an extension of "symbol".

  • Fix infinite directory traversal loop in (*RequireModule).nodeModulesPaths.

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

@nwidger nwidger force-pushed the module-search-algorithm branch from ae6e33a to 5b1ea25 Compare July 15, 2020 16:29
@nwidger
Copy link
Author

nwidger commented Jul 15, 2020

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.

@nwidger nwidger force-pushed the module-search-algorithm branch 2 times, most recently from 9eebb8c to 03f8136 Compare July 18, 2020 16:56
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 the 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 start path for the module search algorithm in the
resolve method.  When require() is called outside of any other
require() calls, resolveStart should be set to the directory of the
currently executing script.  As this information is stored in
r.runtime.Program.src.name but not currently exported, resolveStart is
currently set to ".".  During nested require() calls, resolveStart is
set to the directory of the module 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 dop251#5.
@dop251
Copy link
Owner

dop251 commented Jul 27, 2020

Have a look at 8b60149
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
Copy link
Author

nwidger commented Jul 27, 2020

@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 added a commit that referenced this pull request Jul 27, 2020
@dop251
Copy link
Owner

dop251 commented Jul 27, 2020

All should be fixed now

@nwidger
Copy link
Author

nwidger commented Jul 28, 2020

@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
Copy link
Owner

dop251 commented Jul 28, 2020

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

dop251 added a commit that referenced this pull request Jul 28, 2020
@nwidger
Copy link
Author

nwidger commented Jul 28, 2020

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
Copy link
Author

nwidger commented Jul 29, 2020

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
Copy link
Owner

dop251 commented Aug 4, 2020

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
Copy link
Author

nwidger commented Aug 4, 2020

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

@dop251 dop251 merged commit 03d0f61 into dop251:master Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

require directory not working
2 participants