akofman / cordova-plugin-add-swift-support

:hammer: Swiftify your Cordova app !
MIT License
117 stars 110 forks source link

Changing hook type #3

Closed tabrindle closed 8 years ago

tabrindle commented 8 years ago

Would there be any adverse or unexpected effects of changing the hook type from after_platform_add to after_plugin_add? We have an app with a sizeable amount of code on the native side and its not practical to generate the ios platform on each checkout.

akofman commented 8 years ago

Hello,

Once this plugin installed, it executes a script to check the presence of bridging headers in the other plugins of your project. In order to do that, it must be installed after all other plugins. That's why the script is executed after the platform is added to be sure all other plugins have already been installed. Unfortunately, I tried with an after_prepare hook but it doesn't work with plugins, I'll check that in the Cordova codebase because I think it should work like that.

An after_plugin_add hook would be executed once after adding this plugin. So it won't guaranteed the presence of bridging headers in all other plugins if it is added before them.

In your case, I think the best solution at the moment would be to directly add the add-swift-support script as a hook in your project. Hope it helps.

tabrindle commented 8 years ago

Thank you very much for the quick response. This is pretty much what I expected, and It seems that calling the script directly works. Thanks!

becvert commented 8 years ago

Hi @akofman

an after_prepare hook seems to work but only when you give explicitly the platform to the command

cordova prepare doesn't trigger the hook

cordova prepare ios does trigger the hook

does it make sense? is there a way to fix that?

Thank you

akofman commented 8 years ago

Hey @becvert ! Very helpful comment. Thanks for that. I'm gonna test it and keep you informed. If it's fine, I will update the hook and the README of this plugin and will check with the Cordova team if it is an expected behaviour.

Thanks again.

becvert commented 8 years ago

My guess is that you shoudn't wrap the hook inside the <platform> element

akofman commented 8 years ago

Sounds good ! I just have to check if it doesn't throw any error messages in case of a non-ios platform.

akofman commented 8 years ago

@becvert it's fine to me. If you could have a try with this last commit and confirm everything is also right to you, I'll merge it.

becvert commented 8 years ago

It's fine to me too

akofman commented 8 years ago

Thanks ! Merged in version 1.1.0.

becvert commented 8 years ago

@akofman When a plugin dependent of yours is first installed, its header file doesn't get added to the Bridging Header immediately but only on subsequent prepare.

which is probably not a problem if you do cordova plugin add myplugin and then cordova prepare

but in some instances you install plugins from config.xml by calling cordova prepare once. then it fails. only a subsequent prepare fixes it. Does it make sense?

I believe now that this hook is better:

<hook type="after_plugin_add" src="src/add-swift-support.js" />

sorry for the confusion...

akofman commented 8 years ago

Hmm indeed, I think we could keep both hooks. We'll do that layer. Le 10 juin 2016 12:27 PM, "becvert" notifications@github.com a écrit :

@akofman https://github.com/akofman When a plugin dependent of yours is first installed, its header file doesn't get added to the Bridging Header immediately but only on subsequent prepare.

which is probably not a problem if you do cordova plugin add myplugin and then cordova prepare

but in some instances you install plugins from config.xml by calling cordova prepare once. then it fails. only a subsequent prepare fixes it. Does it make sense?

I believe now that this hook is better:

sorry for the confusion...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/akofman/cordova-plugin-add-swift-support/issues/3#issuecomment-225148307, or mute the thread https://github.com/notifications/unsubscribe/AAjZUqQeXZK9YExVYkIyFlx5czDtxhvFks5qKTv6gaJpZM4INFRA .

becvert commented 8 years ago

that will do :+1: thank you

akofman commented 8 years ago

OK from this branch I added some hooks and updated the script.

I tested multiple cases :

Any objections ?

becvert commented 8 years ago

none :+1: