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

[cordova] add resource-file to mobile-config #9748

Merged
merged 5 commits into from Apr 26, 2018
Merged

[cordova] add resource-file to mobile-config #9748

merged 5 commits into from Apr 26, 2018

Conversation

eskan
Copy link
Contributor

@eskan eskan commented Mar 15, 2018

Hi,
this PR allow to add resource files, see cordova documentation

there is a feature-request meteor/meteor-feature-requests#279

the added function is
App.addResourceFile(src,target,[platform]);

which will be translated in config.xml :

<platform name="android|ios|both">
  <resource-file src="src.file" target="target" />
</platform>

the App.addResourceFile target parameter is mandatory here as the resource will be copied first at the root of the cordova project. Then cordova builder will copy the file to the target destination.

Why resource-file is need ?

some plugin like cordova-plugin-firebase
need configuration file which is use by Google Services.
here is how to set configuration file for this plugin :

  1. Put both google-services.json and GoogleService-Info.plist in your meteor root project
  2. Add these lines to your mobile-config.js, APP_NAME should need to be replaced without the real app name
App.addResourceFile('google-services.json', 'src/google-services.json', 'android');
App.addResourceFile('GoogleService-Info.plist', 'APP_NAME/Resources/GoogleService-Info.plist', 'ios');

this will add

<platform name="ios">
    ...
   <resource-file src="GoogleService-Info.plist" target="APP_NAME/Resources/GoogleService-Info.plist"/>
</platform>
<platform name="android">
    ...
    <resource-file src="google-services.json" target="src/google-services.json"/>
</platform>

to the generated config.xml

What is missing ?

Documentation needs to be updated

@benjamn
Copy link
Contributor

benjamn commented Mar 21, 2018

@eskan This is looking great to me! Will you be able to update the documentation?

@eskan
Copy link
Contributor Author

eskan commented Mar 22, 2018

@benjamn "Pas de soucis"
meteor/docs#260

@etyp
Copy link

etyp commented Mar 31, 2018

@eskan this is awesome. In the meantime, is there another way to include google-services.json with Meteor Cordova override?

@eskan
Copy link
Contributor Author

eskan commented Mar 31, 2018

@etyp unfortunately not.

@etyp
Copy link

etyp commented Mar 31, 2018

Got it. I managed to add the following to config.xml:

<platform name="android">
    ...
    <resource-file src="google-services.json" target="google-services.json"/>
</platform>

And my google-services.json file by including both of them in the cordova-build-override/ directory. It seems like both were successfully included in .meteor/local/cordova-build but my push plugin still won't work.

Not sure if it's a separate issue, but @eskan is there any reason you can think of for that to not be a temporary workaround? (My assumption is that it doesn't work because of some other unrelated issue)

@eskan
Copy link
Contributor Author

eskan commented Mar 31, 2018

hi @etyp, it's not a workaround to me as you need to put/change files in a temporary or autogenerated directory. But it could be a way to test notification with your plugin until this PR will be merged.
I get notifications work with the plugin and the configuration mentioned above.

configureAndCopyResourceFiles(resourceFiles, iosElement, androidElement) {
_.each(resourceFiles, resourceFile => {
// copy file in cordova project root directory
var filename = resourceFile.src.split('\\').pop().split('/').pop();
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, is this splitting/popping on resourceFile.src in order to account for the different file path separators on Windows vs Unix, or is this because of something else I'm missing?

If it is because of such a reason, can we use Node's path.parse instead? A backslash is an uncommon, but possible, character in a valid Unix path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abernix sorry for this ugly thing, i've updated the PR with path.parse()
regards

@abernix
Copy link
Contributor

abernix commented Apr 4, 2018

Please ignore the "waiting" tests. They will go away if you rebase this off the current devel branch and push, but otherwise, it's simply a side-effect of us switching CircleCI configurations.

@hwillson hwillson self-assigned this Apr 18, 2018
@benjamn benjamn added this to the Release 1.7 milestone Apr 18, 2018
@hwillson hwillson self-requested a review April 19, 2018 19:43
Copy link
Contributor

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Thanks @eskan - we should be all set here. LGTM!

@hwillson hwillson removed their assignment Apr 19, 2018
Copy link
Contributor

@abernix abernix left a comment

Choose a reason for hiding this comment

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

LGTM.

@brucejo75
Copy link
Contributor

Hey @eskan, is it possible to address #10684.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants