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

executeFile method - feature request #59

Closed
rbro opened this issue Oct 21, 2013 · 13 comments
Closed

executeFile method - feature request #59

rbro opened this issue Oct 21, 2013 · 13 comments

Comments

@rbro
Copy link
Contributor

rbro commented Oct 21, 2013

Thank you for a very useful extension. Would it be possible to add an executeFile() method that works just like executeString() except it takes a filename as its first argument? This would avoid having to call file_get_contents when the javascript to execute is in an external file.

For example, rather than doing:

$v8->executeString(file_get_contents('test.js'));

executeFile would allow calls like:

$v8->executeFile('test.js');

Thanks.

@satoshi75nakamoto
Copy link
Contributor

I'll take a look at this, but it makes sense to me.

@satoshi75nakamoto
Copy link
Contributor

@cscott — What do you think about adding this feature?

@cscott
Copy link
Collaborator

cscott commented Oct 27, 2013

Sounds reasonable. It would be even more compelling if it was integrated
with the PHP autoloader so it could be smart-ish about paths and maybe
reuse loaded modules. Something like V8Js.require(filename). Because
otherwise it's just saving one method call to file_get_contents, which is a
little bit of convenience but maybe not enough to merit adding extra API.
On Oct 27, 2013 1:37 PM, "Patrick Reilly" notifications@github.com wrote:

@cscott https://github.com/cscott — What do you think about adding this
feature?


Reply to this email directly or view it on GitHubhttps://github.com//issues/59#issuecomment-27174260
.

@rbro
Copy link
Contributor Author

rbro commented Oct 27, 2013

Thanks for your help and considering this request. The other advantage of having an executeFile method (and avoiding the extra call to file_get_contents) is that the raw JS source code doesn't need to be loaded into a PHP string, which may affect the net memory usage used by the script (with memory_get_usage).

@beest
Copy link
Collaborator

beest commented Oct 27, 2013

@cscott @preillyme do you know about the require function that is available from JS?

This functionality could be re-used to provide module loading functionality which follows the CommonJS modules specification - see http://wiki.commonjs.org/wiki/Modules/1.1

Most of the groundwork had been done such as absolute / relative paths and reusing modules.

@cscott
Copy link
Collaborator

cscott commented Oct 28, 2013

wrt saving string storage space -- there's already a (good) suggestion in
the codebase to add a new API function to represent a compiled script.
This would be exported using the persistent PHP functionality, so that
scripts would only be loaded from string and compiled once, and then reused
by subsequent requests. This might also be a compelling reason/time to add
the 'load from file' argument, because we could just recheck the name of
the file (and the timestamp) before reusing the compiled string (instead of
having to load the entire string from the file just to check to see if it
was the same as the previously compiled source).​

@stesie
Copy link
Member

stesie commented Oct 29, 2013

By the way I'm missing a function which can syntax check some javascript source in a string, i.e. compile and throw away afterwards. Maybe this could/should be considered if we decide to introduce the persistent compiled code thing.

The function could be useful if you allow a user to modify some javascript code in the frontend and want to validate it within a save hook.

@cscott
Copy link
Collaborator

cscott commented Oct 30, 2013

There's already a 'require' method exported to JS. Presumably $v8->executeString('require("file.js")') would do what the original requester wanted? If I go through with the refactoring discussed in issue #72, the require method would be PHP-callable, so $v8->getUserContext()->require('file.js') would work from PHP code.

@rbro
Copy link
Contributor Author

rbro commented Oct 31, 2013

I tried running $v8->executeString('require("file.js")'), but I got back a V8JsScriptException:

file:1: No module loader

Does using the require method mean that the included file must be a javascript module?

In my case, my included file contains a list of global functions like:

function test1() { ... }
function test2() { ... }
function test3() { ... }

@beest
Copy link
Collaborator

beest commented Nov 3, 2013

In order to use the JS require function, you need to use the PHP API to provide a module loader. Here's an example:

$v8js->setModuleLoader(function($path) {
    // Load from the given path and return JS code to be executed
});

The point of this callback abstraction is security. I didn't want to give Javascript access to a function that can load and execute code from a file.

Of course, loading and executing from a file may be a common use case of this anyway:

$c8js->setModuleLoader(function($path) {
    return file_get_contents($path);
});

In practice you probably want to perform some sanity checking of the path to ensure that it's resolving to a file and that nothing malicious is going on.

@beest
Copy link
Collaborator

beest commented Nov 3, 2013

Also remember that the require'd files don't share the same global scope as your V8JS code. You need to use the "exports" variable to return an API to be used. See http://wiki.commonjs.org/wiki/Modules/1.1. Here's a really simple example of a module:

exports.add = function(a, b) {
    return a + b;
};

exports.sub = function(a, b) {
    return a - b;
};

Then you require it in your code like this:

var math = require("path/to/math");
var result = math.add(1, 1);

@rbro
Copy link
Contributor Author

rbro commented Nov 3, 2013

Thanks for your help. In my use case, I was just looking for a way to include a js file without having to load it first with file_get_contents - an executeFile method which is the exact equivalent of executeString except to pass it a file path. Similar to how other PHP extensions have functions to load either a string or a file - for example, DOMDocument::load and DOMDocument::loadXML or simplexml_load_file and simplexml_load_string.

@stesie
Copy link
Member

stesie commented Oct 18, 2014

Revisiting this issue almost one year later ... and now having a compileString method as well ... I think we shouldn't add yet another method that just does the file_get_contents and nothing more.

After all we would have to provide executeFile as well as compileFile ... and don't know what not in the future.

Hence I'm now just closing this issue since there was no recent activity anyways.

@stesie stesie closed this as completed Oct 18, 2014
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

No branches or pull requests

5 participants