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

xcode was removed, but is required with 8.1.0 #706

Closed
Tallyb opened this issue Sep 27, 2018 · 10 comments
Closed

xcode was removed, but is required with 8.1.0 #706

Tallyb opened this issue Sep 27, 2018 · 10 comments
Labels

Comments

@Tallyb
Copy link

Tallyb commented Sep 27, 2018

https://github.com/apache/cordova-lib/pull/611/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2L44

errors occur when adding platform ios.
Error: Error: Cannot find module 'xcode'

@brodycj
Copy link

brodycj commented Sep 27, 2018

Can you please give us some more information:

  • result of node --version
  • result of npm --version
  • command you used to add iOS platform

cordova platform add ios works fine for me in my local clone of https://github.com/brodybits/cordova-dialogs-bootstrap-test

cordova run ios --device (with my own package ID) and cordova emulate ios also work fine for me

@Tallyb
Copy link
Author

Tallyb commented Sep 27, 2018

I have created a repo for the error: https://github.com/Tallyb/cordova-xcode-error (based on your repo)
Node 10.11.0, npm 6.4.1 (latest and greatest...)
I have installed cordova locall (npm i -D cordova). I have multiple version that are not compliant, so I must have cordova installed locally. Installed 8.1.0
The problem was with few plugins, one of them was branch-cordova-sdk.
$(npm bin)/cordova platform add ios
Here is the error:

Failed to install 'branch-cordova-sdk': Error: Cannot find module 'xcode'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:581:15)
    at Function.Module._load (internal/modules/cjs/loader.js:507:25)
    at Module.require (internal/modules/cjs/loader.js:637:17)
    at require (internal/modules/cjs/helpers.js:20:18)
    at Context.requireCordovaModule (/Users/tallyb/Documents/dev/cordova-dialogs-bootstrap-test/node_modules/cordova-lib/src/hooks/Context.js:77:12)
    at getProjectModuleGlob (/Users/tallyb/Documents/dev/cordova-dialogs-bootstrap-test/plugins/branch-cordova-sdk/src/scripts/npm/processConfigXml.js:159:31)
    at getProjectModule (/Users/tallyb/Documents/dev/cordova-dialogs-bootstrap-test/plugins/branch-cordova-sdk/src/scripts/npm/processConfigXml.js:145:16)
    at getBranchPreferences (/Users/tallyb/Documents/dev/cordova-dialogs-bootstrap-test/plugins/branch-cordova-sdk/src/scripts/npm/processConfigXml.js:64:25)
    at Object.read (/Users/tallyb/Documents/dev/cordova-dialogs-bootstrap-test/plugins/branch-cordova-sdk/src/scripts/npm/processConfigXml.js:17:31)
    at run (/Users/tallyb/Documents/dev/cordova-dialogs-bootstrap-test/plugins/branch-cordova-sdk/src/scripts/hooks/beforePrepare.js:15:43)
Cannot find module 'xcode'

npm i -S xcode and redo - error is gone (you get team error, but this is because I put a fake id...). You can switch to valid configuration and retest.

@brodycj
Copy link

brodycj commented Sep 27, 2018

I think the explanation is that branch-cordova-sdk uses xcode without explicitly declaring it as a dependency, which happened to work since cordova-lib had the extra xcode dependency. I would be happy to make a workaround solution for Cordova 8.1.x but this extra xcode dependency will definitely not be supported in upcoming Cordova 9. More explanation is coming soon.

@raphinesse
Copy link
Contributor

@brodybits the problem here is that we basically encouraged this kind of thing with Context.requireCordovaModule. At least for Cordova 8, adding the dependency to xcode back would be the right thing to do from a semver perspective. Actually, if we wanted to do it the right way, we would have to restore any other dependency we dropped or changed a version of. These are basically all major semver changes with Context.requireCordovaModule.

Which is why Context.requireCordovaModule is so ridiculously bad and the reason I suggested to deprecate that method a while ago in #689.

@brodycj brodycj changed the title xcode was removed, but is reuqired with 8.1.0 xcode was removed, but is required with 8.1.0 Sep 27, 2018
@brodycj
Copy link

brodycj commented Sep 27, 2018

At least for Cordova 8, adding the dependency to xcode back would be the right thing to do from a semver perspective.

Will do

Actually, if we wanted to do it the right way, we would have to restore any other dependency we dropped or changed a version of. These are basically all major semver changes with Context.requireCordovaModule.

Agreed for any other dependency dropped or major version changed. But for request, while we dropped exact version 2.79.0 I think we should just add request@2 which should not reintroduce any npm audit warnings. Keep in mind that this was a minor release (as opposed to a patch release).

@janpio janpio added the bug label Sep 27, 2018
@Tallyb
Copy link
Author

Tallyb commented Sep 28, 2018

I can see the logic.
Since the workaround is pretty straight forward (npm install ) this can be easily overcome with a deprecation message & documentation.
A clear message of library xyz is relying on package abc which is no longer provided by cordova. Run npm install abc yourself could do magic to cordova developers hours of work and sanity...

@brodycj
Copy link

brodycj commented Sep 28, 2018

Well said @Tallyb, my deepest apologies for such a bad mess-up. I just raised PR #708 with a hotfix, as needed to follow proper semver etiquette (semver/semver#461).

brodycj pushed a commit to brodycj/cordova-lib that referenced this issue Sep 28, 2018
brodycj pushed a commit to brodycj/cordova-lib that referenced this issue Sep 28, 2018
- dependency-ls@1
- unorm@1
- underscore@1
- valid-identifier@0.0.1

needed to adhere to semver principle

ref: apacheGH-706
brodycj pushed a commit to brodycj/cordova-lib that referenced this issue Sep 28, 2018
needed to adhere to semver principles ref: apacheGH-706
brodycj pushed a commit to brodycj/cordova-lib that referenced this issue Sep 28, 2018
brodycj pushed a commit to brodycj/cordova-lib that referenced this issue Sep 28, 2018
- dependency-ls@1
- unorm@1
- underscore@1
- valid-identifier@0.0.1

needed to adhere to semver principle

ref: apacheGH-706
brodycj pushed a commit to brodycj/cordova-lib that referenced this issue Sep 28, 2018
needed to adhere to semver principles ref: apacheGH-706
brodycj pushed a commit that referenced this issue Sep 28, 2018
@deliverymanager
Copy link

In my case it only worked when I did
npm install xcode -g

@raphinesse
Copy link
Contributor

@deliverymanager Yes, if you have a global installation of Cordova you also have to install xcode globally to work around this issue. Thanks for pointing that out.

A patch for this is underway.

@brodycj
Copy link

brodycj commented Oct 7, 2018

This issue is now resolved in cordova-lib@8.1.1 and cordova@8.1.2, closing now. Please keep in mind that we are planning to deprecate this form of usage and completely drop support in a new release as discussed in #689.

We would strongly advise all plugins and applications to explicitly include xcode in dependencies (or devDependencies, if appropriate) and use require('xcode') instead of requireCordovaModule('xcode)` asap.

In case of any questions please do not hesitate to ask via phonegap@googlegroups.com, dev@cordova.apache.org, or in a new Cordova issue (https://github.com/apache/cordova/issues).

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

No branches or pull requests

5 participants