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

fix(android): ignore additional require argument #11378

Closed
wants to merge 1 commit into from

Conversation

janvennemann
Copy link
Contributor

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

@garymathews This is my naive solution, which just drops the additional parameter we currently support in our require on Android. I couldn't find any requires in our codebase that make use of this second parameter.

I'm note sure if this is just a relic from older versions or if we actually still use this. We could change this PR to something like this to avoid breaking changes:

  function require(path, context) {
    if (typeof context !== 'object' || context === null) {
      context = {}
    }
    return self.require(path, context);
  }

@build
Copy link
Contributor

build commented Dec 6, 2019

Fails
🚫 Tests have failed, see below for more information.
Messages
📖

💾 Here's the generated SDK zipfile.

📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖 ❌ 2 tests have failed There are 2 tests failing and 711 skipped out of 7334 total tests.

Tests:

ClassnameNameTimeError
ios.ipad.Titanium.UI.NavigationWindow#popToRootWindow (13.0)10.002
Error: timeout of 10000ms exceeded
file:///Users/build/Library/Developer/CoreSimulator/Devices/1ABD275D-943B-4670-BCE9-89440C554A15/data/Containers/Bundle/Application/DE0B971B-0C64-43C7-95B6-71E067E23C0E/mocha.app/ti-mocha.js:6535:53732
ios.iphone.Titanium.UI.TableViewDelete row (Search Active) (13.0)5.23
Error: timeout of 5000ms exceeded
file:///Users/build/Library/Developer/CoreSimulator/Devices/E1914280-4286-4FB6-9D85-C732FE9BCF33/data/Containers/Bundle/Application/D3224668-7B4A-424D-A104-6582A5A1E891/mocha.app/ti-mocha.js:6535:53732

Generated by 🚫 dangerJS against 4ea6f31

@sgtcoolguy
Copy link
Contributor

Looks to me like we pass around this stuff in the context:

  • currentTab
  • currentService
  • currentTabGroup
  • global
  • module
  • sourceUrl

The module/sourceUrl are set by the module/js require/load code anyways.

The interesting ones are currentTab, currentTabGroup and currentService:

  • I don't see where we're actually setting a value for currentTab or currentTabGroup. I suspect they may be broken?
  • currentTabGroup isn't even documented.
  • currentService appears to be passed in via Module.runModule via V8Runtime java/cpp layer

I suspect that the require should probably not take a second argument at all and should inherit the parent module's context with overridden module/sourceUrl values.

I'll see if I can come up with something....

@sgtcoolguy
Copy link
Contributor

Note that currentTab doesn't appear to be implemented on iOS. I suspect it may have been removed long ago but the docs never deprecated/removed it.

@sgtcoolguy
Copy link
Contributor

...and that Ti.Android.currentActivity and rootActivity are implemented in java code.

@jquick-axway
Copy link
Contributor

This will break the Ti.Android.currentService property.

This property is not defined in Java code. It's defined in JavaScript.
How the code currently works is every required-in JS file gets its own copy of the Ti.Android module. A service JS file referenced in the "tiapp.xml" (ex: <service url="MyService.js"/>) will have its copy of the Ti.Android module assigned a Java ServiceProxy to the Ti.Android.currentService property. Each service JS will reference a different Java ServiceProxy instance since they can be ran in parallel. And the currentService property will be undefined for non-service scripts.

Pretty much everyone who uses Android services use the currentService property. It is a popular feature. Especially for devs who want to collect location data in the background. Example code which leverages this property can be found here...
#11197

I agree that this is not a great design. Changing it isn't trivial though.

Copy link
Contributor

@jquick-axway jquick-axway left a comment

Choose a reason for hiding this comment

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

This will break the Ti.Android.currentService property.

@sgtcoolguy
Copy link
Contributor

I'm going to kill this one off in favor of #11380, since this naive approach seems like it'll break Ti.Android.currentService and there's a lot of collateral changes I think that should be made around this stuff.

@sgtcoolguy sgtcoolguy closed this Dec 6, 2019
@jquick-axway
Copy link
Contributor

Hmm... maybe I'm wrong. We do have a mocha unit test which tests the currentService property below.
https://github.com/appcelerator/titanium-mobile-mocha-suite/blob/master/Resources/ti.android.service.normal.js

I think it's not failing because we only apply the ServiceProxy as a 2nd argument when we load the service JS from the Java side. No one requires-in a service JS directly. It's been a year since I've played around with this code, so, my memory is a bit fuzzy on the exact details. I would still want to run various tests to be sure though.

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