Titanium JIRA Archive
Titanium SDK/CLI (TIMOB)

[TIMOB-16153] CLI: Hook system has wrong callback signatures and poor error handling

GitHub Issuen/a
TypeBug
PriorityHigh
StatusClosed
ResolutionFixed
Resolution Date2014-01-17T01:36:41.000+0000
Affected Version/sRelease 3.2.0
Fix Version/s2014 Sprint 01, 2014 Sprint 01 Core, Release 3.2.1, Release 3.3.0
ComponentsCLI
Labelsqe-testadded
ReporterChris Barber
AssigneeChris Barber
Created2014-01-09T03:14:44.000+0000
Updated2014-09-15T14:48:52.000+0000

Description

The hook system has a big issue where the "pre" function hook signature differs from the "post". This is a bug. They should be the same and the "post" is correct, the "pre" is not. Both the "pre" and "post" should have the first argument be a possible error object. The second arg should be the "data" object. The hook system only processes errors for post hooks, not pre hooks. So, by fixing the "pre" function signature, fixing up error handling to prevent the hook from finishing should be easy. This shouldn't impact plugins or the SDK with these changes since "pre" hooks are used often. The fire hook callback (the calls from the SDK) current emit 3 args, though only 2 make sense. We should fix this, but this will break 3.2.0. This can be fixed by checking the callback function length. Lastly, the if a pre or post function hook should fail, then all places in the SDK that fire a hook should look for errors and gracefully abort the operation.

Attachments

FileDateSize
batman.zip2014-01-21T21:02:19.000+00002731

Comments

  1. Chris Barber 2014-01-10

    Titanium CLI master pull request: https://github.com/appcelerator/titanium/pull/94 Titanium CLI 3.2.x pull request: https://github.com/appcelerator/titanium/pull/100 Titanium SDK master pull request: https://github.com/appcelerator/titanium_mobile/pull/5206 Titanium SDK 3.2.x pull request: https://github.com/appcelerator/titanium_mobile/pull/5207 To test: 1. create an app 2. unzip the attached batman.zip into you project dir 3. edit tiapp.xml and add enable the plugin: 
       <plugins>
       <plugin>batman</plugin>
   </plugins>
    4. build an app for ios and android 5. watch the output for the various pre and post log messages To test the error handling: 1. edit the plugin: plugins/batman/hooks/robin.js 2. locate the "build.android.javac" hook listener 3. change the "pre" function's next(null, data); to next(true, data); 4. build an android app 5. observe that the hook has returned an error and caused the build to fail
  2. Tony Lukasavage 2014-01-10

    I remember talking about this, or something very similar, WRT the Soasta hook. We should make sure that this change causes no problems with the newly rewritten Soasta hook, as handled in TIMOB-15914.
  3. Chris Barber 2014-01-16

    [~tlukasavage] I just checked and the Soasta hook looks to be unaffected by these changes.
  4. Chris Barber 2014-01-17

    Here's the PRs for Tizen: https://github.com/appcelerator/titanium_mobile_tizen/pull/5 https://github.com/appcelerator/titanium_mobile_tizen/pull/6
  5. Chris Barber 2014-01-18

    Found a small regression in the iOS build when building for iOS device. https://github.com/appcelerator/titanium_mobile/pull/5236 https://github.com/appcelerator/titanium_mobile/pull/5237
  6. Olga Romero 2014-01-20

    Tested following test steps in [~cbarber] comment and verified the 
       [FUNCTION-POST-HOOK] 
   [FUNCTION-PRE-HOOK]
    are the same for iOS and Android in the console, however Tizen fails 
       TypeError: Object 6 has no method 'replace'
       at /Users/oromero/Library/Application Support/Titanium/mobilesdk/osx/3.2.1.v20140117222448/tizen/cli/commands/_build.js:851:79
       at Array.forEach (native)
       at build.assembleTitaniumJS (/Users/oromero/Library/Application Support/Titanium/mobilesdk/osx/3.2.1.v20140117222448/tizen/cli/commands/_build.js:847:22)
       at /Users/oromero/Library/Application Support/Titanium/mobilesdk/osx/3.2.1.v20140117222448/node_modules/async/lib/async.js:508:21
       at /Users/oromero/Library/Application Support/Titanium/mobilesdk/osx/3.2.1.v20140117222448/node_modules/async/lib/async.js:224:13
       at /Users/oromero/Library/Application Support/Titanium/mobilesdk/osx/3.2.1.v20140117222448/node_modules/async/lib/async.js:108:13
       at Array.forEach (native)
       at _each (/Users/oromero/Library/Application Support/Titanium/mobilesdk/osx/3.2.1.v20140117222448/node_modules/async/lib/async.js:32:24)
       at async.each (/Users/oromero/Library/Application Support/Titanium/mobilesdk/osx/3.2.1.v20140117222448/node_modules/async/lib/async.js:107:9)
       at _asyncMap (/Users/oromero/Library/Application Support/Titanium/mobilesdk/osx/3.2.1.v20140117222448/node_modules/async/lib/async.js:223:9)
    ├── acs@1.0.11 ├── alloy@1.3.1-beta2 ├── npm@1.3.2 ├── titanium@3.2.1 └── titanium-code-processor@1.1.0 npm -g ls titanium: /usr/local/lib └── titanium@3.2.1 (git://github.com/appcelerator/titanium.git#93286e7b263a968cb362cf4e2a367a9b31f4ccb3)
  7. Olga Romero 2014-01-21

    Tested and verified the fix, using the corrected batman.zip Appcelerator Studio, build: 3.2.1.201401201818 Titanium SDK, build: 3.2.1.v20140121105646 ├── acs@1.0.11 ├── alloy@1.3.1-beta2 ├── npm@1.3.2 ├── titanium@3.2.1 └── titanium-code-processor@1.1.0 npm -g ls titanium: /usr/local/lib └── titanium@3.2.1 (git://github.com/appcelerator/titanium.git#93286e7b263a968cb362cf4e2a367a9b31f4ccb3) No errors
  8. Chris Barber 2014-02-04

    To recap, Titanium CLI 3.2.0 and older, you would do: 
       exports.init = function (logger, config, cli, appc) {
       cli.on('build.android.copyResource', {
           pre: function (data, next) {
               logger.log('[FUNCTION-PRE-HOOK] build.android.copyResource'.magenta);
               next(data); // no error param :(
           },
           post: function (data, next) {
               logger.log('[FUNCTION-POST-HOOK] build.android.copyResource'.magenta);
               next(null, data);
           }
       });
   };
    Starting in Titanium CLI 3.2.1 and newer, the "pre" callback expects the callback to pass back an error argument: 
       exports.init = function (logger, config, cli, appc) {
       cli.on('build.android.copyResource', {
           pre: function (data, next) {
               logger.log('[FUNCTION-PRE-HOOK] build.android.copyResource'.magenta);
               if ((new Date).getMonth() == 1) {
                   // stop the build
                   next(new Error("We don't work in February!"));
               } else {
                   next(null, data);
               }
           },
           post: function (data, next) {
               logger.log('[FUNCTION-POST-HOOK] build.android.copyResource'.magenta);
               next(null, data);
           }
       });
   };
    To support all CLI versions, then you'd have to check the version number like this: 
       exports.init = function (logger, config, cli, appc) {
       cli.on('build.android.copyResource', {
           pre: function (data, next) {
               logger.log('[FUNCTION-PRE-HOOK] build.android.copyResource'.magenta);
               if ((new Date).getMonth() == 1) {
                   // stop the build
                   if (appc.version.gte(appc.version.format(cli.version, 0, 3, true), '3.2.1')) {
                       next(new Error("We don't work in February!"));
                   } else {
                       throw new Error("We don't work in February!")
                       // or process.exit(1);
                   }
               } else {
                   if (appc.version.gte(appc.version.format(cli.version, 0, 3, true), '3.2.1')) {
                       next(null, data);
                   } else {
                       next(data);
                   }
               }
           },
           post: function (data, next) {
               logger.log('[FUNCTION-POST-HOOK] build.android.copyResource'.magenta);
               next(null, data);
           }
       });
   };

JSON Source