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

Supporting cordova-cli@9 or cordova-lib@9 #1034

Open
wants to merge 73 commits into
base: master
Choose a base branch
from

Conversation

sagrawal31
Copy link

As per cordova-lib@9, the requireCordovaModule can only be used to get the cordova-* dependency. This PR is to support that change.

For more info, apache/cordova-lib#706

@ilyakamens
Copy link

ilyakamens commented Apr 1, 2019

Would love for this to be merged.

Edit: I've forked it and made the same change in the meantime.

ilyakamens added a commit to propelinc/cordova-plugin-firebase that referenced this pull request Apr 1, 2019
@forgr-owner
Copy link

Would love it too !

@mismith
Copy link

mismith commented Apr 3, 2019

@sagrawal31 can you get the build passing?

@sagrawal31
Copy link
Author

sagrawal31 commented Apr 3, 2019 via email

@sagrawal31
Copy link
Author

@mismith I fixed the Travis build. I think, all of them was from before this MR.

@mismith
Copy link

mismith commented Apr 3, 2019

Nice work! @arnesson could this be merged now?

@BobCashStory
Copy link

i tryed your with cordova plugin add https://github.com/wizpanda/cordova-plugin-firebase pr and it work well !

@@ -9,7 +9,8 @@ ADDITIONAL_PLUGIN_1=$4
ADDITIONAL_PLUGIN_2=$5
ADDITIONAL_PLUGIN_3=$6

bash ./test/platform-add.sh $CORDOVA_VERSION $PLATFORM $PLATFORM_VERSION
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this what made the build pass? If yes, I'll submit a separate PR with this fix, so all the broken PRs would start passing after rebasing.

Copy link
Author

Choose a reason for hiding this comment

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

Not really, all but yes, it did fixed a few test cases. Since the cordova & platform version passed to this script from the parent script are same hence there was no point reinstalling it again so I commented out that. But I would love to see the detailed error when this script was trying to install the same cordova & platform.

Copy link
Author

Choose a reason for hiding this comment

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

I would also love to help you on rebasing PRs & getting them merged which are good to go because I saw various good PRs

Choose a reason for hiding this comment

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

how can we go forward ?

Copy link
Author

Choose a reason for hiding this comment

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

@felipeclopes can tell anything

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not really sure hot to proceed. It seems that the first step would be to try to fix the test cases that are broken. If you look at the list of PRs to merge, the only ones passing are the ones who removed the tests for older Cordova versions and this one.

Maybe a separate PR just with this fix of the build cold help the other PRs to rebase and get a green status on the build system.

Copy link
Author

Choose a reason for hiding this comment

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

In my thinking, we can start with merging this branch to master then start rebasing other PR with master so most of the PRs will by default pass

Copy link
Author

Choose a reason for hiding this comment

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

@felipeclopes any update? I'll still suggest to move forward with this

@pcova2099
Copy link

pcova2099 commented Apr 15, 2019

Hi, I installed the plugin today (2019-04-15) but the error still happening (Failed to install 'cordova-plugin-firebase': CordovaError: Using "requireCordovaModule" to load non-cordova module "xcode" is not supported. Instead, add this module to your dependencies and use regular "require" to load it.)

    <plugin name="cordova-plugin-firebase" spec="https://github.com/arnesson/cordova-plugin-firebase" />
    <engine name="android" spec="7.1.4" />
    <engine name="ios" spec="4.5.5" />

Version 2.0.5 was installed by cordova

@forgr-owner
Copy link

forgr-owner commented Apr 15, 2019

@pcova2099 it's not merged that why, if you want to make it work before merge, use this cordova plugin add https://github.com/wizpanda/cordova-plugin-firebase

@pcova2099
Copy link

pcova2099 commented Apr 15, 2019

I´ve got this error with

cordova plugin add https://github.com/wizpanda/cordova-plugin-firebase

Installing "cordova-plugin-firebase" for ios
Failed to install 'cordova-plugin-firebase': Error: Cannot find module 'xcode'
Ionic:

   ionic (Ionic CLI)  : 4.4.0 (C:\Users\pcova\AppData\Roaming\npm\node_modules\ionic)
   Ionic Framework    : ionic-angular 3.9.3
   @ionic/app-scripts : 3.2.1

Cordova:

   cordova (Cordova CLI) : 8.1.2 ([email protected])
   Cordova Platforms     : ios 4.5.5
   Cordova Plugins       : cordova-plugin-ionic-keyboard 2.1.3, cordova-plugin-ionic-webview 3.1.2, (and 5 other plugins)

System:

 (C:\Program Files (x86)\Android\android-sdk)
   NodeJS            : v8.11.1 (C:\Program Files (x86)\nodejs\node.exe)
   npm               : 5.6.0
   OS                : Windows 10

@sagrawal31
Copy link
Author

sagrawal31 commented Apr 16, 2019

@pcova2099 you need to run npm i xcode --save-dev in your project as a temporary workaround as I have mentioned here

rammie pushed a commit to propelinc/cordova-plugin-firebase that referenced this pull request Apr 21, 2019
@gdespiritorflex
Copy link

+1

@hiepxanh
Copy link

hiepxanh commented May 12, 2019

hello sir, I see this PR along time, can I ask, when this will be merge?

@remisture
Copy link

Is this project dead?

@paulsmith47
Copy link

I am also waiting for this PR to be merged will this happen any time soon?

@nascarjake
Copy link

This is still marked open and I'm still getting the error, assuming this isn't merged yet. Do we know when it will be? Have yet to try the work around. Wanted to know how close the merge was.

@ryanmtaylor
Copy link

@nascarjake a lot of people have moved to https://github.com/wizpanda/cordova-plugin-firebase-lib or https://github.com/dpa99c/cordova-plugin-firebasex

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.