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

GH-706 dependency patch fix for 8.x only #708

Merged
merged 8 commits into from Sep 28, 2018

Conversation

@brodybits
Contributor

brodybits commented Sep 28, 2018

UPDATED:

  • reintroduce xcode dependency as needed in 8.1.x to avoid breaking plugins such as branch-cordova-sdk before next major release, to resolve GH-706 (bug #706)
  • emit a message in case someone uses requireCordovaModule('xcode') that it is now deprecated, will be removed in the near future, along with the recommended solution
  • reintroduce some other dependencies to better ensure that any other plugins or applications using requireCordovaModule would not be broken by a minor release upgrade
  • tar dependency back to tar@2
  • emit a deprecation warning message in case a plugin or application uses requireCordovaModule with a non-Cordova module
  • updates from PR #707:
    • minor optimization for requireCordovaModule for module names that do not start with "cordova-lib"
    • JSDoc updates
    • Context#requireCordovaModule unit tests

This fix is NOT WANTED in master branch. I think we do not want to support requireCordovaModule('xcode') in the next major release.

This fix was tested as follows:

  • pushed this change to bug-706-xcode-hotfix-test on my fork
  • made bug-706-xcode-hotfix-test branch on my fork of cordova-cli that uses cordova-lib from git+https://github.com/brodybits/cordova-lib.git#bug-706-xcode-hotfix-test as dependency
  • In https://github.com/brodybits/cordova-xcode-error (fork of https://github.com/Tallyb/cordova-xcode-error with config.xml fix):
    • I used cross-platform nvs tool (https://github.com/jasongin/nvs) to switch to Node.js 10.10 and do npm i -g https://github.com/brodybits/cordova-cli#bug-706-xcode-hotfix-test
    • cordova platform add ios outputs the deprecation message, does not fail with the xcode error

Resolves #706

@brodybits brodybits requested review from janpio and raphinesse Sep 28, 2018

@project-bot project-bot bot added this to new/unassigned/reopened prs in Apache Cordova: project-bot automation testing Sep 28, 2018

@project-bot project-bot bot added this to 🐣 New PR / Untriaged in Apache Cordova: Tooling Pull Requests Sep 28, 2018

@janpio

This comment has been minimized.

Contributor

janpio commented Sep 28, 2018

(PR title should describe the change first, then administrative information)

@brodybits brodybits changed the title from GH-706 xcode hotfix for 8.1.x only to GH-706 xcode dependency hotfix for 8.1.x only Sep 28, 2018

@raphinesse

This comment has been minimized.

Contributor

raphinesse commented Sep 28, 2018

I think a minor release would be more fitting to introducing a deprecation notice, but I'm not quite sure. What would you say?

@raphinesse

This comment has been minimized.

Contributor

raphinesse commented Sep 28, 2018

To really adhere to semver, we cannot drop any dependencies or make major version updates because of #689. So here's the list of dependencies that we dropped between 8.0.0 and 8.1.0:

dependency-ls@^1.1.1
request@2.79.0
unorm@1.4.1
valid-identifier@0.0.1
xcode@^1.0.0

Furthermore, we updated tar from 2.2.1 to ^4.4.1.

@brodybits

This comment has been minimized.

Contributor

brodybits commented Sep 28, 2018

I just pushed some changes:

  • updates from #707
  • squash fixes to support Node.js 4 (unsure if 'use strict' is needed or not)
  • fix const pkg declaration for Node.js 4 & resolve unused variable in eslint
  • remove extra xcode warning that we added, due to the more general deprecation warning from #707 (squash fix)

Coming up are changes to restore the dependencies we dropped in 8.1.0.

Show resolved Hide resolved src/hooks/Context.js Outdated
@raphinesse

This comment has been minimized.

Contributor

raphinesse commented Sep 28, 2018

@brodybits ping me when you think this is ready to review, please.

@janpio janpio changed the title from GH-706 xcode dependency hotfix for 8.1.x only to [WIP] GH-706 xcode dependency hotfix for 8.1.x only Sep 28, 2018

@janpio janpio moved this from 🐣 New PR / Untriaged to 👷 Blocked: Work in Progress in Apache Cordova: Tooling Pull Requests Sep 28, 2018

@brodybits brodybits changed the title from [WIP] GH-706 xcode dependency hotfix for 8.1.x only to [WIP] GH-706 xcode dependency patch fix for 8.1.x only Sep 28, 2018

@brodybits brodybits removed the request for review from janpio Sep 28, 2018

@brodybits brodybits changed the title from [WIP] GH-706 xcode dependency patch fix for 8.1.x only to GH-706 xcode dependency patch fix for 8.1.x only Sep 28, 2018

@brodybits

This comment has been minimized.

Contributor

brodybits commented Sep 28, 2018

@raphinesse this PR is now ready for review.

@@ -24,6 +24,7 @@
"cordova-js": "^4.2.2",
"cordova-serve": "^2.0.0",
"dep-graph": "1.1.0",
"dependency-ls": "^1.1.1",

This comment has been minimized.

@janpio

janpio Sep 28, 2018

Contributor

this and all the other dependencies that are readded are not reflected in the PR title and description

@raphinesse

LGTM. The only thing I'm still unsure about is whether this shouldn't rather be a semver minor change, because of the deprecation.

Apache Cordova: Tooling Pull Requests automation moved this from 👷 Blocked: Work in Progress to ✅ Approved, waiting for Merge Sep 28, 2018

@project-bot project-bot bot moved this from new/unassigned/reopened prs to accepted prs in Apache Cordova: project-bot automation testing Sep 28, 2018

@brodybits brodybits changed the title from GH-706 xcode dependency patch fix for 8.1.x only to [WIP] GH-706 dependency patch fix for 8.1.x only Sep 28, 2018

@janpio

This comment has been minimized.

Contributor

janpio commented Sep 28, 2018

It's just a deprecation notice, right? Then it should be acceptable to include it in a patch release.

@brodybits brodybits changed the title from [WIP] GH-706 dependency patch fix for 8.1.x only to GH-706 dependency patch fix for 8.1.x only Sep 28, 2018

@brodybits

This comment has been minimized.

Contributor

brodybits commented Sep 28, 2018

It's just a deprecation notice, right?

Yes. And I would like to resolve the GH-706 in a patch release.

One more thing is that if cordova-lib and Cordova CLI do not have the same version number there some extra "noise" will show up in cordova --version like this:

8.1.2-dev (cordova-lib@8.1.1-dev)

I would favor publishing this in cordova-lib@8.1.2 to avoid such noise.

@janpio

This comment has been minimized.

Contributor

janpio commented Sep 28, 2018

I would favor publishing this in cordova-lib@8.1.2 to avoid such noise.

That's not how semver works.
That "noise" is expected and should be there (actually not showing the cordova-lib version if they are equal is not optimal).

brodybits and others added some commits Sep 28, 2018

raphinesse and others added some commits Sep 28, 2018

Require fix for module names that do *not* start with "cordova-lib"
(quick optimization)

Co-authored-by: Raphael von der Grün <raphinesse@gmail.com>
Co-authored-by: Christopher J. Brody <chris.brody@gmail.com>
'use strict' for hooks context - 8.x only
as needed to support Node.js 4
Add more dependencies from 8.0.0 for 8.x only
- dependency-ls@1
- unorm@1
- underscore@1
- valid-identifier@0.0.1

needed to adhere to semver principle

ref: GH-706
Back to npm i tar@2 in dependencies - 8.x only
needed to adhere to semver principles ref: GH-706
@brodybits

This comment has been minimized.

Contributor

brodybits commented Sep 28, 2018

As discussed on the #dev channel on Slack, I just removed the changes to emit deprecation warning messages in case of non-Cordova modules (also removed a test case). This is to avoid the issue with https://semver.org/#spec-item-7 where a minor version increase is needed in case of the deprecation message. Merging now.

@brodybits brodybits changed the title from GH-706 dependency patch fix for 8.1.x only to GH-706 dependency patch fix for 8.x only Sep 28, 2018

@brodybits brodybits merged commit 24e8545 into apache:8.1.x Sep 28, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

Apache Cordova: Tooling Pull Requests automation moved this from ✅ Approved, waiting for Merge to 🏆 Merged, waiting for Release Sep 28, 2018

@project-bot project-bot bot moved this from accepted prs to merged prs in Apache Cordova: project-bot automation testing Sep 28, 2018

@brodybits brodybits deleted the brodybits:GH-706-xcode-hotfix branch Sep 28, 2018

@janpio

This comment has been minimized.

Contributor

janpio commented Sep 28, 2018

You didn't actually remove the code before merging.

@brodybits

This comment has been minimized.

Contributor

brodybits commented Sep 28, 2018

What I removed was cherry-pick of 27a2cfd from PR #707. You should not see any trace of it since I did a rebase with the cherry-pick of 27a2cfd. If I look at https://github.com/apache/cordova-lib/pull/708/files the code to emit the deprecation warning is no longer there.

@janpio

This comment has been minimized.

Contributor

janpio commented Sep 28, 2018

Why is the rest of #707 required?
(Don't forget to update the PR description to reflect what you actually merged)

@raphinesse

This comment has been minimized.

Contributor

raphinesse commented Sep 28, 2018

It does fix a theoretical bug but it's definitely not required.

For the record: I'd still prefer to have all the changes I originally approved together in a minor release.

@brodybits

This comment has been minimized.

Contributor

brodybits commented Sep 28, 2018

(Don't forget to update the PR description to reflect what you actually merged)

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment