Titanium JIRA Archive
Titanium SDK/CLI (TIMOB)

[TIMOB-6033] Android: Heavyweight root window causes incorrect CommonJS global variable behavior

GitHub Issuen/a
TypeBug
PriorityLow
StatusClosed
ResolutionWon't Do
Resolution Date2020-01-09T19:27:59.000+0000
Affected Version/sRelease 1.7.5
Fix Version/sn/a
ComponentsAndroid
Labelsdr-list, parity
ReporterIvan Skugor
AssigneeIngo Muschenetz
Created2011-11-05T02:07:09.000+0000
Updated2020-01-09T19:27:59.000+0000

Description

Comments

  1. Paul Dowsett 2011-11-06

    Well spotted, Ivan! Thank you for raising this issue, as there is certainly inconsistent behavior when using a heavyweight root window. I have modified the ticket to make the test case clearer. Please note that I have changed the way you created a heavyweight window, from using the modal to fullscreen property, because the root window by its nature is modal. Cheers
  2. Ivan Skugor 2011-11-07

    Thanks for accepting my ticket, Paul. But I do mind some of your modifications: * "win2" seems unnecessary here (bug is present without it) * events should be attached to the window before it opens. In theory, window could open before event listener is attached to it (maybe it's not a problem for "click" event, but could be for "open" event) * it's not clear how global variables function with CommonJS approach in Titanium. What if variable is declared without "var" keyword in module scope? Will it become global variable or local variable in module scope? Is there a mechanism how to introduce new global variables from module scope? Also, I do need to note that this is like forth ticket that I reported related to the inconsistency between lightweight and heavyweight windows in Android, so I'd liked to bring attention to that to devs working on Android.
  3. Paul Dowsett 2011-11-07

    Ivan Just as you pointed out, the test case was indeed unnecessarily long, so I have improved it. Points to note: * the root window, by its nature, is a modal window, and so the modal property is rarely used for it in real-world applications. Therefore, I have removed this property and added fullscreen, which produces the same result as your original code * to avoid any ambiguity about when the event is invoked, I have changed the open event listener to a click event listener. Again, this produces the same result * eventListener code should be placed after the window is opened, to improve the user experience by reducing the delay until the screen is rendered Be assured that we are aware that there needs to be more information about the use of CommonJS with Titanium, and are working on documentation and video tutorials to address it Thanks for the ticket.
  4. Ivan Skugor 2011-11-07

    Paul, * I don't know for others, but I use modal property for all windows in my (Android) application. OK, maybe it's not necessary for root window, because it's modal by it's nature, but I find that strange because setting modal property to different values has affect as you can see in my example (if root window is modal by it's nature, setting modal property to it should have affect, right?). * hmmm, okay :) * delay shouldn't be present if UI is on separate thread, right? Even if that is not so, "addEventListener" function is extremely fast. On the other hand (if I understand correctly, and please correct me if I'm wrong), window opening is asynchronous, but execution of JS statements is synchronous. That means that window would "open" (maybe not visually because of "asynchronous" opening, but it would "open" from JS statement point of view) and "open" event listener at that time would not be present, therefore event would not be fired to that listener. So ...
       win.open(); //window opens right here from statement point of view, no event listeners attached
       
       //at this time window is already opened from statement point of view, so it would not be fired if window opening would be super-fast
       win.addEventListener('open', function() {
       
       });
       
    Sometimes your approach will work because window opening is slow (and of course, asynchronous) and JS execution is fast. To be precise, if time necessary for window to open is greater than time needed to execute all statements including "addEventListener", then your approach will work. If that is no so, it will not work. Check this example:
       var rootWin = Ti.UI.createWindow({
       	fullscreen: true,
       	backgroundColor: '#0f0'
       });
       
       rootWin.open();
       
       rootWin.addEventListener('open', function() {
       	
       	Ti.API.info('Root window opened ');
       	
       	var win = Ti.UI.createWindow({
       		//modal: true,
       		backgroundColor: 'magenta'
       	});
       	
       	win.open();
       	
       	var doNothing = [];
       	for (var i = 0; i < 100; ++i) {
       		doNothing.push('Doing nothing ...' + i);
       	}
       	
       	win.addEventListener('open', function() {
       		Ti.API.info('Window opened ');
       	});
       	
       });
       
    (I had to use "magenta" as background color to illustrate the disaster, sorry about that) For some reason example doesn't work for root window (only dear God and Don Thorp knows why), but you can see the potential disaster for non-root window if there is some delay introduced before adding of event listener (beside "magenta" background color, there is no console output "Window opened"). I hope you understand the problem now and that you won't correct my code that way (again!). :) (if you need more explanation, please let me know)
  5. Rick Blalock 2011-11-17

    To add to this discussion: In general, globals in a CommonJS setting are considered bad. In fact NodeJS completely disallows them. You get a few like an object called 'global' that you can append things to but NodeJS kills anything else in a global context. This is common across many CommonJS implementations. I'm not sure what will be implemented in our V8 version of Android but here are some notes: 1. Don't use globals. If you need to pass things (like an an app object to manage state, etc.) pass that in to your module. This is common practice amongst the Node community as well. On iOS you don't have to and globals will be present. This isn't guaranteed to always work though. Safest bet is to always pass in to your module what the module will use. 2. As far as I can tell this is how it works on iOS currently: Globals can be brought in to the module's scope. If you overwrite the global in the module scope - it doesn't overwrite it globally IF you overwrite it using 'var'. e.g.
       // Global
       var myapp = {};
       var mymodule = require('somemodule');
       
       // somemodule.js
       var myapp = 'cool beans';
       // Now myapp inside 'somemodule.js' is 'cool beans' but it is not changed in the global context (and there's no way to access it in the module now
       
    I've seen this change throughout releases but I'm pretty sure that's how it works currently. At the end of the day: DONT USE GLOBALS inside a commonjs module. Pass things in to it. 3. I think Android is following a similar path to NodeJS for the V8 implementation. So you'll want to do what I'm recommending above for long term support. Marshall am I correct on that?
  6. Ivan Skugor 2011-11-18

    Hi Rick, thanks for your comment. I agree that, in general, global variables should be avoided, but sometimes they are useful. For example, if there is some common functionality that you don't want to re-include in every module (because it's against DRY principle). If CommonJS specification allows global variables, then I don't see a reason why Titanium should disallow them. I didn't find any decent explanation about global variables in CommonJS (although, many CommonJS implementations allows them in different ways), anyone can share some knowledge? I'm not sure do I understand you correctly regarding point 2., but ... I think that's the way it should work. If variable is declared using "var" keyword in some particular scope, then it's local variable to that scope and not global scope, even if variable with same name is declared in global scope. See this example:
       //this.c === window.c === global object
       var c = 5;
       	
       window.onload = function() {
       	var c = 2;
       	alert(c);
       	alert(window.c);
       };
       
    This alerts "2" and "5". I think module scope is pretty similar. If you define variable in module scope using "var" keyword, then it's local variable in that module, even if it is defined in global scope. If it's defined without "var" keyword, JS engine will try to find it in super-scope (I don't know right word :) ). So ...
       // Global scope
       var myapp1 = {};
       var myapp2 = {};
       var mymodule = require('somemodule');
       
       // somemodule.js
       //global module scope
       var myapp1 = 'cool beans'; //doesn't change global scope variable because it's local variable
       myapp2 = 'cool beans'; // affects global scope variable
       
    This is how it works on Android when there are no heavyweight windows involved (I don't know how iOS side functions). That approach is pretty JavaScript-ish, but the problem is that the problem of polluting global scope is not solved because (accidentally) declaring variable without "var" keyword would implicitly create new global variable. I think some implementations use "global" as global variable to avoid that. In them global variables must be set explicitly using "global.variableName = ...". IMHO, the problem is not implementation, it's programmer's bad practice (declaration of variable without "var" keyword). Using "global" variable does not solve that problem, lousy practice is actually encouraged (because now programmer can declare variable without "var" keyword without consequences).
  7. Ivan Skugor 2011-12-02

    Kevin seems to like globals. :) https://github.com/appcelerator-developer-relations/Forging-Titanium/blob/master/ep-006/SCTabGroup/Resources/ui/AppWindow.js#L14 Anyway, just to note that this issue does not exist when lightweight windows are used, I think that behaviors in LW & HW windows should be the same.
  8. Kevin Whinnery 2011-12-02

    Most rich client app technologies I've worked with (Flex/WPF/Browser) allow for a global scope. This is a useful tools for doing things like caching and binding to data. I think we should provide a consistent means for using globals, regardless of whether Ti.include or require is used.
  9. Marshall Culpepper 2011-12-02

    Moving forward, Window URLs and CommonJS modules should be seen as functionally equivalent in their access to global scope. (We actually implement Window URLs as CommonJS modules in our new V8 implementation). That being said, objects on the global scope should in principle never be used (or be allowed to be set) by a CommonJS module. It's true that some CommonJS implementations allow this, but it is not specifically spelled out by the spec, and in my opinion goes against the spirit of CommonJS. In Android, starting with our new V8 implementation, each module is purposefully sandboxed into a new, *private* context which has it's own private global. As Rick stated above, there is never a technical reason to clobber globals from within the private scope of a module -- if you need to reuse components, then those components can be passed in as part of an initialization process, i.e...
       // app.js
       var myExpensiveThing = //...
       
       var myModule = require("myModule");
       myModule.init(myExpensiveThing);
       
       // myModule.js
       exports.init = function(expensiveThing) {
         // ...
       }
       
    The fact that clobbering globals works currently in Window URLs / CommonJS modules should be considered a bug, as this is not desired behavior moving forward.
  10. Kevin Whinnery 2011-12-02

    Just had a conversation with Opie - it turns out that in the new CommonJS implementation (and possibly before, I don't know), modules themselves are stateful, which is a departure from how I had been thinking about modules. If modules can have persistent global object, then the need for an app-wide global state is removed. Let's say I had some global config options I needed to store. I could have a module to store my app's state which looked like this:
        var STATE = {
        	username:'kwhinnery' //some default value
        };
        
        exports.get = function(key) {
        	return STATE[key];
        };
        
        exports.set = function(key, value) {
        	STATE[key] = value;
        };
        
    Which could be consumed across my entire app like so:
        var appstate = require('module/defined/above');
        var username = appstate.get('username');
        
    If this is uniformly implemented on the iOS side as well, then I can't think of a legitimate use case for having global variables.
  11. Ivan Skugor 2012-01-11

    Kevin's example does not show that module is stateful (since "username" is hardcoded, so it will always return same value), to show that module is stateful, "username" has to be changed, and that change should be reflected on re-required module. Anyway, just to add more info if someone bumps to this issue ... module scope creates new JS context, similar to what "iframe" is in browser. That means - clear JS environment. So, if you change JS native prototype (which are by default global objects), that won't be reflected in module's scope:
        //app.js
        String.prototype.say = function() {};
        var someString = 'Meh';
        Ti.API.info(typeof someString.say); // function
        
        //module.js
        var modulesString = 'Meh';
        Ti.API.info(typeof modulesString.say); // undefined
        
    One more reason not to mess with native prototypes. :)
  12. Kevin Whinnery 2012-01-11

    Good point on the prototypes, but the module objects are indeed stateful. If you use the "set" function in my example, as of 1.8, the object will be updated for all users of the module.
  13. Ivan Skugor 2012-01-11

    Yes, I agree that modules are stateful, but (IMHO) your example missed to show that.
        //somewhere
        var appstate1 = require('module/defined/above');
        var username1 = appstate1.get('username'); //kwhinnery
        appstate1.set('username', 'iskugor');
        
        
        //somewhere else
        var appstate2 = require('module/defined/above');
        var username2 = appstate2.get('username'); //iskugor
        
    Didn't mean to say it's wrong, only that it may not be clear enough at first glance. Also, (just for info) this module's "statefulness" brings some potential problems. If module's API has some method and someone overrides it, then it will be reflected on all module's instances (it's actually one instance because modules are singletons). Sometimes that's not what we want.
  14. Alan Hutton 2020-01-09

    It has been decided that this issue should be closed as “Won’t do.” This issue is out of date with our current supported SDK release (7.5.2.GA as of the date of closure), and out of date with mobile OS versions. Updating, or creating code may not reproduce the issue reported, or be a valid test case. If community members feel that the issue is still valid, please create a new ticket. Please reference this closed ticket number, include SDK used, comments, and code that demonstrates/reproduces the issue.

JSON Source