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.

Sign up for GitHub

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

Jump to bottom

TIMOB-5123 Use Rhino native require() CommonJS implementation #493

Merged
merged 12 commits into from
Sep 28, 2011
Merged

TIMOB-5123 Use Rhino native require() CommonJS implementation #493

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1cccac7
TIMOB-5123 Use Rhino's native "require()" implementation.
billdawson Sep 23, 2011
85e0dd4
TIMOB-5123 Cache require()'d scripts just like other scripts
billdawson Sep 23, 2011
a508517
TIMOB-5123 Updated smalljs.jar with commonjs classes
billdawson Sep 23, 2011
a92bc7a
Merge branch 'appc_master' into timob-5123
billdawson Sep 23, 2011
ed5436e
Merge branch 'appc_master' into timob-5123
billdawson Sep 26, 2011
adfccf3
TIMOB-5123 (from pull request feedback) remove unnecessary casting in…
billdawson Sep 27, 2011
4388db8
TIMOB-5123 Don't unnecessarily require that result of require() be a …
billdawson Sep 27, 2011
c5c9db2
TIMOB-5123 Don't force require() to return KrollProxy. Only do that i…
billdawson Sep 27, 2011
5cf22ff
Merge branch 'appc_master' into timob-5123
billdawson Sep 27, 2011
29ea073
TIMOB-5123 Don't mess with the results of Rhino require(). Pass it on!
billdawson Sep 27, 2011
8746e2c
TIMOB-5123 move the buffer/stream close() to a finally{}, where it sh…
billdawson Sep 27, 2011
076cb69
Merge branch 'appc_master' into timob-5123
billdawson Sep 28, 2011
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Binary file modified android/titanium/lib/smalljs.jar
View file
Open in desktop
Binary file not shown.
25 changes: 25 additions & 0 deletions android/titanium/src/org/appcelerator/titanium/TiScriptRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

import org.appcelerator.titanium.util.Log;
import org.mozilla.javascript.Context;
import org.mozilla.javascript.NativeFunction;
import org.mozilla.javascript.RhinoException;
import org.mozilla.javascript.Script;
import org.mozilla.javascript.Scriptable;
Expand Down Expand Up @@ -86,6 +87,30 @@ public void setAppPackageName(String packageName)
appPackageName = packageName;
}

public Script getScript(Context context, Scriptable scope, String relativePath)
throws ClassNotFoundException, InstantiationException, IllegalAccessException
{
String scriptClassName = getScriptClassName(relativePath);
TiScript tiScript = scripts.get(scriptClassName);
if (tiScript != null) {
return tiScript.script;
}
Class<?> scriptClass = Class.forName(scriptClassName);
tiScript = new TiScript();
tiScript.context = context;
tiScript.scope = scope;
if (scriptClass != null) {
@SuppressWarnings("unchecked")
Class<NativeFunction> scriptClaxx = (Class<NativeFunction>) scriptClass;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the NativeFunction specialization (and @SuppressWarnings) necessary here? you're casting directly to Script on the line below

Script script = (Script) scriptClaxx.newInstance();
tiScript.script = script;
tiScript.name = scriptClass.getName();
scripts.put(tiScript.name, tiScript);
return script;
} else {
throw new ClassNotFoundException("CommonJS module class for \"" + relativePath + "\" not found.");
}
}

public Object runScript(Context context, Scriptable scope, String relativePath)
throws ClassNotFoundException
Expand Down
67 changes: 67 additions & 0 deletions android/titanium/src/org/appcelerator/titanium/kroll/KrollContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,13 @@
import org.mozilla.javascript.Context;
import org.mozilla.javascript.EcmaError;
import org.mozilla.javascript.EvaluatorException;
import org.mozilla.javascript.Script;
import org.mozilla.javascript.Scriptable;
import org.mozilla.javascript.ScriptableObject;
import org.mozilla.javascript.commonjs.module.ModuleScript;
import org.mozilla.javascript.commonjs.module.ModuleScriptProvider;
import org.mozilla.javascript.commonjs.module.Require;
import org.mozilla.javascript.commonjs.module.RequireBuilder;

import android.app.Activity;
import android.os.Handler;
Expand All @@ -54,6 +59,7 @@ public class KrollContext implements Handler.Callback
private static KrollThreadListener threadListener;

private KrollHandlerThread thread;
private Require commonJsRequire;
private TiContext tiContext;
private ScriptableObject jsScope;
private String sourceUrl;
Expand Down Expand Up @@ -189,12 +195,73 @@ protected void initContext()
if (DBG) {
Log.d(LCAT, "Initialized scope: " + jsScope);
}
this.commonJsRequire = buildCommonJsRequire(ctx);
if (DBG) {
Log.d(LCAT, "Initialized commonJS require() function: " + this.commonJsRequire);
}
initialized.countDown();
} finally {
exit();
}
}

private Require buildCommonJsRequire(Context ctx)
{
RequireBuilder builder = new RequireBuilder();
builder.setModuleScriptProvider(new ModuleScriptProvider()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is ModuleScriptProvider an interface? it may be better to implement directly on KrollContext to save ourselves yet another class to dex :)

{
@Override
public ModuleScript getModuleScript(Context context, String moduleId,
Scriptable paths) throws Exception
{
Script script = null;
String uri;

StringBuilder sb = new StringBuilder();
sb.append(TiC.URL_APP_PREFIX)
.append(moduleId)
.append(".js");
uri = sb.toString();

if (useOptimization) {
// get Script from compiled script class
script = TiScriptRunner.getInstance()
.getScript(context, jsScope, moduleId);
if (script == null) {
Log.e(LCAT, "Could not retrieve a Script object for module '" + moduleId + "'.");
Context.throwAsScriptRuntimeEx(new Exception("Unable to load Script for module '" + moduleId + "'."));
}
} else {
// make Script from JS source
TiBaseFile file = TiFileFactory.createTitaniumFile(tiContext, new String[] { uri }, false);
BufferedReader br = null;
try {
br = new BufferedReader(new InputStreamReader(file.getInputStream()), 4000);
script = context.compileReader(br, uri, 1, null);
br.close();
} catch (IOException e) {
Log.e(LCAT, "IOException reading module file: " + uri, e);
Context.throwAsScriptRuntimeEx(e);
}
}

return new ModuleScript(script, uri);
}
});

return builder.createRequire(ctx, jsScope);
}

public Object callCommonJsRequire(String path)
{
Context ctx = enter();
try {
return commonJsRequire.call(ctx, jsScope, jsScope, new String[] { path });
} finally {
exit();
}
}

protected void threadEnded()
{
if (threadListener != null) {
Expand Down
99 changes: 42 additions & 57 deletions android/titanium/src/ti/modules/titanium/TitaniumModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,9 @@
import org.appcelerator.kroll.annotations.Kroll;
import org.appcelerator.titanium.TiApplication;
import org.appcelerator.titanium.TiBaseActivity;
import org.appcelerator.titanium.TiBlob;
import org.appcelerator.titanium.TiC;
import org.appcelerator.titanium.TiContext;
import org.appcelerator.titanium.TiLaunchActivity;
import org.appcelerator.titanium.io.TiBaseFile;
import org.appcelerator.titanium.io.TiFileFactory;
import org.appcelerator.titanium.kroll.KrollCallback;
import org.appcelerator.titanium.kroll.KrollContext;
import org.appcelerator.titanium.kroll.KrollCoverage;
Expand Down Expand Up @@ -443,71 +440,59 @@ public KrollProxy require(KrollInvocation invocation, String path)
return module;
}

// NOTE: CommonJS modules load absolute to app:// in Titanium
if (DBG) {
Log.d(LCAT, "Attempting to include CommonJS module: " + path);
}

builder.setLength(0);
builder.append(TiC.URL_APP_PREFIX)
.append(path)
.append(".js");
String fileUrl = builder.toString();
TiBaseFile tbf = TiFileFactory.createTitaniumFile(ctx, new String[]{ fileUrl }, false);
if (tbf == null) {
//the spec says we are required to throw an exception
Context.reportError("Couldn't find module: " + path);
return null;
}

if (DBG) {
Log.d(LCAT, "Attempting to include JS module: " + tbf.nativePath());
}
try {
TiBlob blob = (TiBlob) tbf.read();
if (blob == null) {
Log.e(LCAT, "Couldn't read required file: " + fileUrl);
return null;
}
// create the CommonJS exporter
KrollProxy proxy = new KrollProxy(ctx);

// TODO: we need to switch to the Rhino native require()
// implementation, but in the meantime this will have to do

// create the CommonJS exporter
KrollProxy proxy = new KrollProxy(ctx);
// call the actual require() implementation.
Object result = null;
try {
result = ctx.getKrollContext().callCommonJsRequire(path);
} catch (Exception e) {
builder.setLength(0);
builder.append("(function(exports){")
.append(blob.getText())
.append("return exports;")
.append("})({})");

Object result = ctx.evalJS(builder.toString());

if (!(result instanceof Scriptable)) {
builder.setLength(0);
builder.append("Module did not correctly return an exports object: ")
.append(path)
.append(", result: ")
.append(result);
Context.throwAsScriptRuntimeEx(new Exception(builder.toString()));
return null;
}

Scriptable exports = (Scriptable) result;
// CommonJS modules export all functions/properties as
// properties of the special exports object provided
for (Object key : exports.getIds()) {
String propName = key.toString();
proxy.setProperty(propName, exports.get(propName, exports));
}
builder.append("require(\"")
.append(path)
.append("\") failed: ")
.append(e.getMessage());
String msg = builder.toString();
Log.e(LCAT, msg, e);
Context.throwAsScriptRuntimeEx(new Exception(msg));
}

// spec says you must have a read-only id property - we don't
// currently support readonly in kroll so this is probably OK for now
proxy.setProperty(TiC.PROPERTY_ID, path);
// uri is optional but we point it to where we loaded it
proxy.setProperty(TiC.PROPERTY_URI, fileUrl);
return proxy;
} catch (Exception ex) {
Log.e(LCAT, "Error loading module named: " + path, ex);
Context.throwAsScriptRuntimeEx(ex);
if (!(result instanceof Scriptable)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't "exports" actually be any old type? (not just an object?) i.e:

var x = require("x");

// x.js
module.exports = 1;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good puzzle to work on. :) I'd never seen code like that (module.exports = as opposed to extending the free-variable exports.) The spec doesn't seem to mention that at all, but indeed if I run it in node, then the value of the variable x becomes just 1.

Our call to Rhino's require(), however, is anyway returning a Scriptable even with that code in the module. Could be Rhino is non-compliant here. I tried a "real" Rhino example as well (where their require() is inserted in the scope, unlike in our use of it.) See here. Again, in that, an empty object is returned, not the value 1.

So I guess Jeff put this check in because he wanted to be sure to be able to use .getIDs. He seems intent on returning a KrollProxy as well, so it will end up being an object (the proxy) in the JS anyway.

Nevertheless, this check seems unnecessary as you say. And I'm not sure it's necessary that we absolutely return a KrollProxy -- wonder why he did that? So I'll get rid of the check and change the return type to Object so a boxed number would end up being possible (even though Rhino's require is not returning that). If indeed a Scriptable comes back from Rhino (which seems to always happen no matter what), I'll go ahead and continue Jeff's pattern of making that into a KrollProxy.

builder.setLength(0);
builder.append("Module did not correctly return an exports object: ")
.append(path)
.append(", result: ")
.append(result);
Context.throwAsScriptRuntimeEx(new Exception(builder.toString()));
return null;
}

Scriptable exports = (Scriptable) result;
// CommonJS modules export all functions/properties as
// properties of the special exports object provided
for (Object key : exports.getIds()) {
String propName = key.toString();
proxy.setProperty(propName, exports.get(propName, exports));
}

// spec says you must have a read-only id property - we don't
// currently support readonly in kroll so this is probably OK for now
proxy.setProperty(TiC.PROPERTY_ID, path);
// uri is optional but we point it to where we loaded it
proxy.setProperty(TiC.PROPERTY_URI, fileUrl);
return proxy;

}

@Kroll.method
Expand Down