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

[7_5_X][TIMOB-26499]: Remove namespace restriction in item templates #10410

Merged
merged 3 commits into from Oct 31, 2018

Conversation

janvennemann
Copy link
Contributor

@janvennemann janvennemann commented Oct 30, 2018

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

Removes the restriction for only Ti or Titanium as a namespace to allow third-party modules again in list view item templates.

Tries to construct the class first, catches the exception that is thrown if the class does not exists and only then falls back to the commonjs module loading.

fixes regression introduced in #10131

DebugLog(@"[DEBUG] Failed to load native class %@, trying Alloy widget / CommonJS module", viewTemplate.type);
NSString *code = [NSString stringWithFormat:@"var result;"
"try {"
" var jsModule = require('/alloy/widgets/%@/controllers/widget');"
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated, but should this be either:

require('./alloy/widgets/%@/controllers/widget');
require('alloy/widgets/%@/controllers/widget');

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it should. The first is a relative URL, the second is a legacy-style path that we should eventually not support (as it was treated as either relative or absolute, and I think differed by platform as to which is tried first).

The alloy directory should be a to-level dir under the app's Resources, so the absolute path should be correct to load it.

@garymathews garymathews changed the title fix(ios): remove namespace restriction in item templates [7_5_X][TIMOB-26499]: Remove namespace restriction in item templates Oct 30, 2018
Copy link
Contributor

@garymathews garymathews left a comment

Choose a reason for hiding this comment

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

CR: PASS
FT: PASS

NOTE: You will need to update the av.imageview config module.xcconfig so its CURRENT_MODULE_VERSION is 1.5.0. This is an issue with the module.

Copy link
Contributor

@sgtcoolguy sgtcoolguy left a comment

Choose a reason for hiding this comment

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

LGTM

"try {"
" var jsModule = require('/alloy/widgets/%@/controllers/widget');"
" if (!jsModule) {"
" jsModule = require('%@');"
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I can see, require throws JS exception instead of returning null nor undefined (tested on iOS emulator). So I think requiring wrong Alloy widget never fallback to CommonJS module here.

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

5 participants