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

chore: update install engines #685

Merged
merged 3 commits into from
Jun 4, 2020
Merged

Conversation

jcesarmobile
Copy link
Member

master version was bumped to 4.0.0-dev, so the package.json should have a new "5.0.0" entry or will fail to install once released.

Updated the "4.0.0" to require cordova-ios >=6.0.0-dev since the ios breaking changes will only work in that new version.

Also updated the engine in plugin.xml for also require cordova-ios >=6.0.0-dev.

Kept the cordova >=3.1.0 as nothing was changed to require a newer version, but maybe we should require at least 9?

Copy link
Member

@NiklasMerz NiklasMerz left a comment

Choose a reason for hiding this comment

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

cordovaDependencies is new to me, but changes make sense. 👍

@NiklasMerz
Copy link
Member

Tests fail with:

  cordova-plugin-inappbrowser-tests.tests >>

    cordova.InAppBrowser

      ✗ inappbrowser.spec.1 should exist

        - Expected undefined to be defined.

Maybe the dependencies cannot be loaded because the -dev versions are not on NPM?

@jcesarmobile
Copy link
Member Author

Yeah, since this requires cordova-ios 6, test won’t work until it’s released. But better have it here so we don’t forget.

@jcesarmobile
Copy link
Member Author

jcesarmobile commented May 14, 2020

Looks like I was wrong and cordova-ios 6 is not required for IAB to work, master is working fine on 5.1.1.
So changing the requirement to 4.0.0 as some 4.0.0 code checks were removed and older versions won't work, but should work on 4+.

@jcesarmobile jcesarmobile requested a review from NiklasMerz May 14, 2020 12:08
Copy link
Member

@NiklasMerz NiklasMerz left a comment

Choose a reason for hiding this comment

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

Quick test was good, too.

@NiklasMerz NiklasMerz merged commit 04ce789 into apache:master Jun 4, 2020
@jcesarmobile jcesarmobile deleted the update-engines branch June 4, 2020 14:57
@@ -43,6 +43,10 @@
"cordova": ">=3.1.0"
},
"4.0.0": {
"cordova": ">=3.1.0",
Copy link

Choose a reason for hiding this comment

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

I think minimum Cordova version should be increased as well, both here and in plugin.xml. I don't think we want to keep supporting old Cordova versions:)

Copy link
Member

Choose a reason for hiding this comment

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

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.

3 participants