search

Heyzap / heyzap-cordova

Heyzap SDK for Apache Cordova
https://www.heyzap.com/
10 stars 18 forks source link

Remove global requires #15

Closed HernanZh closed 7 years ago

HernanZh commented 7 years ago

This pr replaces require with cordova.require because it can clash with other require systems.

I also changed play-services 9.4.0 to latest (+) I guess this is open for debate, because it's a big problem with other cordova plugins. Other plugins can choose any version and that results in compilation errors.

ekilah commented 7 years ago

I'm not familiar with this, is it still the case that this change would be better?

ekilah commented 7 years ago

Your PR has a gradle file that references an older version of the plugin that used the unified SDK (sdk 10), but the current plugin does not. If the two cordova.require statements are what would be helpful for this issue, can the PR only contain those? (or I can just add these separately w/o a PR if that's easier).

My main concern is I don't know what the debate would be to have over this different version of the require statement - I am not a cordova developer by trade, I'm just managing the wrapper for our SDK.

HernanZh commented 7 years ago

I updated the pr (I forgot to reply). The gradle was a leftover from the old Heyzap version and is no longer a concern. I removed it.

The global require statement can cause a conflict with RequireJS, which also defines a global require variable. This is also more consistent, since the other files use "cordova.require". See HeyzapAds.js for example.

ekilah commented 7 years ago

Great, thanks for the updates and explanation @HernanZh - this was built by someone else so I don't have all the context. Merging #15 .