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

[TIMOB-24404] Expose global object on iOS #9552

Merged
merged 2 commits into from Dec 14, 2017

Conversation

janvennemann
Copy link
Contributor

@janvennemann janvennemann commented Oct 24, 2017

JIRA: https://jira.appcelerator.org/browse/TIMOB-24404

Optional Description:
Exposes the global object as a node like global variable. Allows the following code to be run:

global.testVar = 1;
console.log(testVar); // prints "1" to the log

This already works in Alloy apps in Android, but not in classic apps.

@janvennemann janvennemann added this to the 7.0.0 milestone Oct 24, 2017
@janvennemann janvennemann changed the title [TIMOB-24404] Expose global object [TIMOB-24404] Expose global object on iOS Oct 24, 2017
@build
Copy link
Contributor

build commented Oct 25, 2017

Fails
🚫

🔬 There are library changes, but no changes to the unit tests. That's OK as long as you're refactoring existing code, but will require an admin to merge this PR. Please see README.md#unit-tests for docs on unit testing.

Generated by 🚫 dangerJS

@hansemannn
Copy link
Collaborator

@sgtcoolguy I'd like to add the unit-tests for Jan - should I create a new test-suite, e.g. "globals.test.js" or integrate into an existing one? Eventually, we could also test other globals like setTimeout, setInterval, __filename and __dirname.

@hansemannn
Copy link
Collaborator

hansemannn commented Oct 25, 2017

@janvennemann I am not sure about the performance-impact of this change, because (for now) we hang every Ti.* element on the global namespace now, which can be referenced from everywhere now. This example:

var win = Ti.UI.createWindow({
  backgroundColor: '#fff'
});

var btn = Ti.UI.createButton({
  title: 'Trigger'
});

btn.addEventListener('click', function(e) {
  console.log(global);  
});

win.add(btn);
win.open();

returns:

{
    L = "<KrollCallback: 0x60c000266900>";
    alert = "<KrollCallback: 0x60c0002669c0>";
    btn = "[object TiUIButton]";
    clearInterval = "<KrollCallback: 0x60c0002667c0>";
    clearTimeout = "<KrollCallback: 0x60c000266880>";
    console = "[object TiConsole]";
    require = "<KrollCallback: 0x60c000266a00>";
    setInterval = "<KrollCallback: 0x60c000266b00>";
    setTimeout = "<KrollCallback: 0x60c000266bc0>";
    win = "[object TiUIWindow]";
}

I mean, we just reference them, but I feel like this may have some ugly side-effect that should be well-tested against.

@sgtcoolguy
Copy link
Contributor

@hansemannn We have some tests of globals int he suite, but they're poorly organized. Right now they live in:

I'd copy and update one of those, or combine them into a global.test.js.

@hansemannn
Copy link
Collaborator

Ok, I'll remove the ones from ti.internal.test.js as they are in ti.builtin.test.js already, remove ti.internal.test.js, rename ti.builtin.test.js to ti.globals.test.js and add more tests around them. @janvennemann I'll likely cherry-pick your PR and provide it in an own PR.

@janvennemann
Copy link
Contributor Author

@hansemannn, this should not change the scope of any variables, it only makes the global object available under the global var. The reason the button and window variable show up in that list is because every variable declared in app.js is considered a global variable since this is the entry point for the js context. Variables in other files are scoped to their module and will not be available via global.

@sgtcoolguy Support for this on Android seems to only work under Alloy. Any idea why?

@sgtcoolguy
Copy link
Contributor

@janvennemann uhhh, I don't know. Maybe an artifact of the difference in the way we treat app.js versus all other required JS files?

In non-app.js we wrap in a self-invoking function ala Node: https://github.com/appcelerator/titanium_mobile/blob/master/android/runtime/common/src/js/kroll.js#L120

app.js is treated specially: https://github.com/appcelerator/titanium_mobile/blob/master/android/runtime/common/src/js/module.js#L580

@sgtcoolguy
Copy link
Contributor

So to follow up on this:
I'm actually needing this to get #9512 working with ti-mocha. There is no "global" variable/property in iOS, which is causing issues there, where it attempts to preserve some global functions. There are platform difference between iOS and Android so I can't even try and do an if(!global) guard before it re-defines global.

As part of that and this PR I looked at what Node does, and it is exactly what Jan does here. For Android, global works on anywhere but app.js, because in other files we wrap the file in a self invoking function that passes in the global object as global. In app.js we just execute the script in context without a self-invoking function wrapper to pass in things like kroll or global.

To unify the platforms, I think we should hang "global" off the global object as Jan does here, then global should be available everywhere in any JS file for both platforms. We could then remove the global argument to the self-invoking wrapper function on Android.

On another related note, I'm seeing some undocumented/unofficial global weirdness with require too. You can do this.myNewProperty = 1; to affect the global namespace right now in a required file. With #9512 that behavior breaks as it inlines undefined to replace this. Not sure if we care or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants