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

Issue is cordova build error of my app created cdp boilerplate #10

Merged
merged 1 commit into from
Feb 9, 2018

Conversation

Hiroyuki-Anzai
Copy link
Contributor

My app was created by "cdp create mobile" command of cdp-cli.
I commited these sources of the condition that it was created to GitHub.

I cloned cource from GitHub to local and execute the following command with current directory of the sources.

  1. npm installl
  2. npm run build:debug
  3. cordova build android (or cordova build ios)

The cordova build is error.
Because there are no "/platforms//cordova/node_modules".

So I modified sources of cdp-lib as pull request.
Please confirm the following:

  • Added not ignore path to "/templates/mobile/_gitignore" into cdp-lib.

And incidentally, I modified the following item.

  • Added "/" to top of some ignore path.

Best regard.

- Added "/" to top of some ignore path
- Added not ignore path of node_modules used cordova build
@Hiroyuki-Anzai
Copy link
Contributor Author

Additional comment

There were no problems after the confirmation with branch of pull request on the win/mac.

@Shin-Ogata
Copy link
Member

PR ありがとうございます。
しかし既定で cordova の platforms 以下の node_modules をあえて追加できるべきか否かは議論の余地があると思います。

npm_modules を配置するために、$ cordova platform update はネイティブのソースコードも消してしまうリスクを考慮すると利用するのに躊躇がありました。

現時点で私が思うベストな解は sony/cdp-js/packages/cafeteriaにあるように、
npm install 後に、root の npm_modules/cordova-android 以下をコピーもしくはシンボリックリンクすることが良いように思います。

@ahirun0426
Copy link
Member

Shin-Ogata さんの意見は尤もです。
Cordova が platform add する際にも cordova-{platform}\node_modules をコピーするとのこと。
この考え方からするなら、cdp-lib においてもコピーするのが良いのかもしれません。

但し、下記の点から今回は .gitignore に対して node_modules を追記することで回避してよいのではないかと考え、本 PR を merge したいと思います。
・cdp-js のテンプレートが作成する環境は cordova platform add した後の環境
・cordova アプリケーションをビルドする環境としては node_modules が既存であることが期待値
・cordova が node_modules をコピーするという仕様は cordova の思想であり、例えば cordova-platform 以下で npm install を実行するというような adhoc workaround はそれに則らない
⇒ cdp-lib にコピーするコードを用意しておくのも同様

その点、node_modules を git repository に保持しておくというのは、上記思想には反しません。

アプリケーションプロジェクトのレポジトリには、ビルドするのに十分なファイル群は用意されていても良いのではないでしょうか。

@ahirun0426 ahirun0426 merged commit f8a39d9 into master Feb 9, 2018
@ahirun0426 ahirun0426 deleted the issue_cordova_build_error branch February 9, 2018 04:44
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