-
Notifications
You must be signed in to change notification settings - Fork 441
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi!
Thank you very very much for your contribution! This config is a pain for users and once this PR is merged you've made their work a lot easier 👍
Can you please take a look at my remarks?
Thanks!
Eddy
} | ||
} | ||
|
||
if (process.argv.indexOf("config") == -1 && fs.existsSync(pluginConfigPath)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should read process.argv.indexOf("config") > -1
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, there are now two npm scripts in the plugin now. One of them is the postinstall that will check for configuration json and does the configuration. The other is "config" which is run using npm run config
in the plugin folder and provides the "config" arg to the same script the post install is running. It will force the script to ignore the configuration.json and re-ask the configuration questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes I see what you mean 👍
For Android you will not get the title and body if the notification was received while the application was in background, but you will get the *data* payload. | ||
Add the following services in the `app/App_Resources/Android/AndroidManifest.xml` to enable advanced FCM messaging: | ||
``` | ||
<manifest ... > |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! Do you think we'd be able to add this to the postinstall script? Not as part of this PR, just would like to know if you think it would be a wise thing to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is ok for the plugins to require some modifications to be done by hand, as long as these modifications can be kept in source control and are well documented.
If the changes happen to be in platforms/* then it would be best if there is an automated way to apply them (using hooks?) so the app can roundtrip source control and continuous integration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@@ -38,3 +38,5 @@ dependencies { | |||
// compile "com.google.android.gms:play-services-auth:9.6.+" | |||
|
|||
} | |||
|
|||
apply plugin: "com.google.gms.google-services" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly on my machine this leads to:
* Where:
Script '/Users/eddyverbruggen/sandboxes/_remove/firebasepanayot/platforms/android/configurations/nativescript-plugin-firebase/include.gradle' line: 42
* What went wrong:
A problem occurred evaluating script.
> Plugin with id 'com.google.gms.google-services' not found.
I'm running tns 2.3.0.. perhaps this changed in a more recent version?
Still applying the plugin at the bottom of platforms/android/build.gradle
works fine though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'com.google.gms.google-services' plugin still has to be added by hand in the build.gradle
, but the apply plugin
is now part of the firebase include.gradle.
The 'com.google.gms.google-services' can be somewhat automated using an additional after prepare hook like the one here:
https://github.com/NativeScript/nativescript-marketplace-demo/blob/push-notifications/hooks/after-prepare/firebase.build.gradle.js
But it looks far to fragile for me to push this in the firebase plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you double check if it works when you add by hand this in platform/android:
dependencies {
classpath "com.android.tools.build:gradle:X.X.X"
classpath "com.google.gms:google-services:3.0.0"
}
Without the:
apply plugin: "com.google.gms.google-services"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, adding apply plugin: "com.google.gms.google-services"
to the bottom is no longer required. Cool!
I see what you mean with the after prepare hook but I think it's worth the brittleness to include it in the plugin. Worst case scenario the developer still needs to add the classpath by hand but generally this would make installation lots easier.
Merged! Much much better now, thanks so much! |
Btw, still considering that https://github.com/NativeScript/nativescript-marketplace-demo/blob/push-notifications/hooks/after-prepare/firebase.build.gradle.js script for further automation. |
@PanayotCankov Decided to add your "fragile" gradle script. I'll see how it goes, but it's a major improvement that no manual config is required anymore. |
Automate enabling iOS 10 keychain sharing #216
Look forward to trying it! |
The worst case scenario is the naive lookup for the correct place to insert the dependency fails and puts it in a totally wrong place rendering the build script syntactically incorrect making the android build fail miserably with cryptic errors hard to reason about. :) Since you merged it I will probably remove it from the https://github.com/NativeScript/nativescript-marketplace-demo Good news is since we have automated build should something go wrong we will find out in a timely fashion. |
Automate enabling iOS 10 keychain sharing #216
Adding buildscript>dependencies>classpath "com.google.gms:google-services:3.0.0" still has to be done by hand in platforms/android or in hook. We ({N}) will have to modify the CLI to make a copy of build.gradle in app/App_Resources/Android/build.gradle in future.