TIMOB-5123 Use Rhino native require() CommonJS implementation #493
Conversation
| tiScript.scope = scope; | ||
| if (scriptClass != null) { | ||
| @SuppressWarnings("unchecked") | ||
| Class<NativeFunction> scriptClaxx = (Class<NativeFunction>) scriptClass; |
There was a problem hiding this comment.
is the NativeFunction specialization (and @SuppressWarnings) necessary here? you're casting directly to Script on the line below
|
Code review and functional test Accepted |
… TiScriptRunner, make KrollContext implement ModuleScriptProvider rather than making an anonymous instance of it, to save from putting one more class into the dex.
|
updated based on feedback. Allen will need to re-func test. |
|
Sorry, wait to re-review, I haven't addressed the return value issue yet. Will tomorrow. |
…f the call to Rhino require indeed returns an object. Else just return what it gives back.
|
Ready for re-review. Sorry, Allen, but my changes were significant so you'll need to re-test. :) |
|
|
||
| Scriptable exports = (Scriptable) result; | ||
| // CommonJS modules export all functions/properties as | ||
| // CommonJS modules export all functions/properties as | ||
| // properties of the special exports object provided | ||
| for (Object key : exports.getIds()) { |
There was a problem hiding this comment.
I feel like this is overkill -- is there anything wrong with just returning the exports object as is? It's already an object that was created just for the module
|
Ready for re-review. |
|
Code review accepted |
| try { | ||
| br = new BufferedReader(new InputStreamReader(file.getInputStream()), 4000); | ||
| script = context.compileReader(br, uri, 1, null); | ||
| br.close(); |
There was a problem hiding this comment.
Could close br in a finally statement to ensure it gets closed if an exception occurs.
|
Code review and functional test Accepted (see minor comment above) |
…ould have been in the first place. Good catch ayeung!
TIMOB-5123 Use Rhino native require() CommonJS implementation
http://jira.appcelerator.org/browse/TIMOB-5123
Please process also this rhino_titanium pull request:
https://github.com/appcelerator/rhino_titanium/pull/4
To test, simply try the app Kevin mentions in the JIRA item. You should test it both in non-production mode (i.e., just launching to emulator from inside Titanium Studio), to make sure I didn't break anything. Then test in production mode (Distribute - Android in Titanium Studio), which is the mode that wasn't working, to make sure i fixed it.