[TIMOB-7568] CommonJS in Core

[dawsontoth]

Added support for adding CommonJS modules to the core.

[WhichKatieDid]

I'm starting to wonder if we should refactor this so that all of moduleNamed goes straight to Require, do not pass go, do not collect $200. For one, the krollbridge require handles path-based module names while this version does not.

This is something we could revisit later, if needed. Something doesn't smell right about this, but I can't put my finger on it.

[sptramer]

This does require that path always be toplevel, but I think that's expected for pre-compiled JS modules. The real question is whether or not this is intended to be used with pure JS modules that aren't precompiled into binary format - @dawsontoth can you give some input on that?

[dawsontoth]

That was my intention, at least for this pass at solving the problem. Long term, I think it would be great to not have to precompile them to binary. It would be even greater if we could have a single common location for pure JS modules to be loaded by both Android and iOS. But for this pass, it's not necessary at all. Does that answer your question, @sptramer?

[sptramer]

Sure does.

[WhichKatieDid]

I have a few reservations of this implementation, as it seems we're playing fast and loose here.

[dawsontoth]

Yup, I agree. It's also kind of an odd way to include a module... Can you enumerate any serious consequences for this, short term? What do you see as a more long term solution?

It would be nice if there was a common place to dump a CommonJS module, and then both platforms could pick it up and do with it what they want.

[sptramer]

Code reviewed. This looks like an acceptable (for now) implementation for loading compiled CommonJS modules into the Titanium namespace, but we really do need to revisit module inclusion and especially require() infrastructure in the near future.

Some minor changes do need to be made though.

[dawsontoth]

When you say "minor changes" do you mean the KrollObject casting issue you and Blain commented on?

[sptramer]

Yup. Avoid the cast and change the return type to id - it looks like we either throw away the return value (usually) or return it as a generic type itself (in -[TitaniumObject valueForKey:]).

[dawsontoth]

Updated.

[sptramer]

Code reviewed; APPROVED. Will wait for @BlainHamon to CR+FR.

[WhichKatieDid]

I can't put my finger on it, and I think we should streamline the require mechanism in general in the future. Having two completely different points in code where JS can be loaded in seems redundant at best.

Also, I could have sworn addModule had a more important position earlier that required it to be a module. As it is, you're correct in that it's either discarded or immediately fed into a general purpose conversion. CR Accepted

[WhichKatieDid]

So far, FR failed, going to look into what's going on.

[dawsontoth]

Might be due to library not getting added to all targets. I might have missed a step in my testing details in the ticket...

[WhichKatieDid]

Okay, the test has been thus, to simulate, er, simulator. Test.js is a file in resources, not compiled. win.add(require('Test').retrieveSuccessLabel()) is successful. win.add(Ti.Test.retrieveSuccessLabel()) gives an exception about Ti.Test being undefined. Is this a valid use case?

[dawsontoth]

Nope. Check out this comment on the associated Jira ticket:
https://jira.appcelerator.org/browse/TIMOB-7568?focusedCommentId=182374&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-182374

[WhichKatieDid]

It still bothers me that we're introducing an inconsistent mechanism here, that modules can be added in different ways but with different rules, and that the Ti namespace can be modified ad-hoc. Remind me again why we're making such a deep cut into Titanium instead of having the end dev go "Ti.foo = require('foo');"? And why is this called commonJS support, since commonJS's implementation is to use Require?

Either way, there should be explicit documentation, where require('foo') works and Ti.foo does not, and vice versa.

[dawsontoth]

1) Dev should just require('test') the module rather than Ti.Test. This is how CommonJS modules are loaded.
Counter: The module must be a part of the Titanium distribution. That is a decision you must argue with TPTB, not me. So, until you convince them otherwise, we have to be able to Ti.Whatever a JSModule. Also, don't get hung up on the semantics of saying CommonJS. If it helps, think of these as JSModules instead. It's compiled in to a library, and needs to be loadable in the Ti namespace.

2) But then any module can be included via Ti.Test rather than require('test'). That has security and support concerns.
Counter: That's not true. TiHost.m, line 140 will make sure there is a valid class defined for the module. The only way for that to be true is if the module's library is included in the distribution. That limits us to only being able to load the core modules in to the Ti namespace. The only thing we're changing with PR 1373 is to add the capability to load JSModules. Therefore, there aren't security or support concerns here, and we don't need to investigate GUID whitelists or other protection mechanisms.

[WhichKatieDid]

Clarification. I thought this was for commonJS. As Dawson said, this is actually to allow 'compiled JS' titanium modules in the same namespace as native compiled modules. The previous issues remain, as all third party compiled modules already slip into the Ti namespace inadvertently. We're just widening the barn door's opening a little. Anyways, CR+FR PASSED.