Skip to content

Angular*CLI.buildArchitectCommand should not fallback to app #4192

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

Closed
SchnWalter opened this issue Oct 28, 2019 · 8 comments · Fixed by #4349
Closed

Angular*CLI.buildArchitectCommand should not fallback to app #4192

SchnWalter opened this issue Oct 28, 2019 · 8 comments · Fixed by #4349
Labels

Comments

@SchnWalter
Copy link
Contributor

SchnWalter commented Oct 28, 2019

The AngularBuildCLI.buildArchitectCommand() and AngularServeCLI.buildArchitectCommand() methods should not hardcode app as a fallback project name when no project name is provided via arguments. The best approach would be to read the defaultProject key from angular.json or by running ng config defaultProject

With this, the Ionic CLI will be able to work, by default, in Angular workspaces where the defaultProject is not named app; as for the ones generated using the ng add @ionic/angular schematic; see: #4191

L.E. #4121 should be dealt with first.

@ionitron-bot ionitron-bot bot added the triage label Oct 28, 2019
@SchnWalter SchnWalter changed the title buildArchitectCommand should not fallback to app Angular*CLI.buildArchitectCommand should not fallback to app Oct 28, 2019
@imhoffd
Copy link
Contributor

imhoffd commented Feb 5, 2020

@SchnWalter How can I reproduce the error case where it's falling back to app in a multi-app environment? In my test repo, the Ionic CLI properly warns about not knowing which project to use for ionic build, ionic serve, and ionic g.

image

The "bare projects" error isn't the ideal message, but it seems to error properly when it's supposed to. If I try to manipulate ionic.config.json in a way where it treats the project in a way that's both single-app and multi-app, I get this correct error:

image

See my test repo branches:

What am I missing?

@SchnWalter
Copy link
Contributor Author

SchnWalter commented Feb 5, 2020

You need to use Angular CLI to generate a new workspace with a default project that is an Ionic application and some other angular specific libraries and/or applications. To do this I am using the Ionic Angular CLI schematic, which doesn't hardcode app as the project name (which had the same issue but was fixed in ionic-team/ionic-framework#19765 and ionic-team/ionic-framework#20281)

To reproduce this step you can checkout https://github.com/SchnWalter/ionic-cli-issue-4192 (or follow the "Steps for recreating the demo repository" section below.

If you grep for defaultProject in angular.json for a project generated using Ionic CLI, you will see this:

> grep defaultProject angular.json
  "defaultProject": "app"

But if you do the same after you follow the steps bellow (or just clone the linked repository), you will see this:

> grep defaultProject angular.json
  "defaultProject": "my-ionic-client"

To reproduce the issue issue you have to install the latest Ionic CLI (not the modified version)

npm install -g @ionic/cli
ionic --version
6.0.1

And you will see that the ionic commands will fail:

ionic serve
> ng run app:serve --host=localhost --port=8100
[ng] An unhandled exception occurred: Project does not exist.
[ng] See "/tmp/ng-FvRPZs/angular-errors.log" for further details.

[ERROR] ng has unexpectedly closed (exit code 127).

        The Ionic CLI will exit. Please check any output above for error details.

And with the version that is included in the PR that I sent earlier, you will see that the ionic commands will work properly:

ionic serve
> ng run my-ionic-client:serve --host=localhost --port=8100

--

Steps for recreating the demo repository:

cd ~/MyProjects/

# Create a new empty Angular workspace.
ng new --create-application=false --new-project-root='.' my-frontend-monorepo

cd ~/MyProjects/my-frontend-monorepo/

# Generate two minimal applications, one to be use with ionic.
# We want the ionic application to be the defaultProject, so we generate it first.
ng generate application --minimal --prefix=mic --routing --style=css my-ionic-client
git add . && git commit -m "add my-ionic-client application"
ng generate application --minimal --prefix=mwc --routing --style=css my-web-client
git add . && git commit -m "add my-web-client application"

# Now add Ionic support using the partial Angular CLI schematic
ng add @ionic/angular --project=my-ionic-client

And because ng add @ionic/angular doesn't create a fully working ionic project (see the first issue above), you need to ionic.config.json config.xml resources/ from an existing ionic application that was generated using Ionic CLI and also to fix the path to src/theme/variables.css which hasn't been fix in the above MRs (ignore this part: ionic-team/ionic-framework#19904)

cd ~/MyProjects/

# Generate a new temporary Ionic cordova project to extract the missing resources.
ionic start --cordova --type=angular my-ionic-client blank

cd ~/MyProjects/my-ionic-client/

# Copy missing resources to the mono-repository:
# Here I'm using cordova, but I assume that the same would apply to Capacitor.
cp -r config.xml ionic.config.json resources/ ../my-frontend-monorepo

cd ~/MyProjects/my-frontend-monorepo/
git add . && git commit -m "add missing ionic cordova files"

# Delete temporary project
rm -r ~/MyProjects/my-ionic-client/

@SchnWalter
Copy link
Contributor Author

SchnWalter commented Feb 12, 2020

Ionic CLI properly warns about not knowing which project to use

It only works when you have multiple Ionic applications in the same repository. If you have an Angular workspace with multiple projects, but only one Ionic application, as the default project, then you get this error:

> ionic serve
> ng run app:serve --host=localhost --port=8100
[ng] An unhandled exception occurred: Project does not exist.
[ng] See "/tmp/ng-3sQiH7/angular-errors.log" for further details.

[ERROR] ng has unexpectedly closed (exit code 127).

        The Ionic CLI will exit. Please check any output above for error details.

Because there is no Angular project called app in angular.json and because the ionic.config.json doesn't have a projects key with multiple entries.

In multi-app Angular workspaces that contain a single Ionic application, the ionic.config.json will most probably look like the one from the example repository that I linked in the previous comment:

{
  "name": "my-ionic-client",
  "integrations": {
    "cordova": {}
  },
  "type": "angular"
}

imhoffd added a commit that referenced this issue Mar 3, 2020
This flag automates the setup steps for multi-app projects (see the
docs: https://ionicframework.com/docs/cli/configuration#multi-app-projects).

Although this feature was built largely with Angular monorepos in mind,
it is flexible for all project types. See this wiki article for examples
of how to use this feature in Angular: https://github.com/ionic-team/ionic-cli/wiki/Angular-Monorepo

ref: #4121
ref: #4192
@imhoffd
Copy link
Contributor

imhoffd commented Mar 3, 2020

@SchnWalter I added a feature to ionic init which should cover the use case if I'm understanding it correctly.

Please update to Ionic CLI 6.2.0 and see the example in this new wiki article: https://github.com/ionic-team/ionic-cli/wiki/Angular-Monorepo (largely inspired by your example). You can tweak the prefix/ID from app to something else to test that it works.

The issue I have with your PR is that it conflicts with the design decision to decouple the Ionic CLI and Angular CLI as much as possible, which makes things more flexible (for example, you can have an Ionic React and Ionic Angular app in the same multi-app project) and easier to maintain in the future (right now we're not reading/writing to angular.json).

I hope this is acceptable and resolves your issue. I think documentation is a big part of this, which is why I wrote up the wiki article and plan to add examples in the future as well as updated the Multi-app Project docs which are meant to be as general as possible.

Thank you so much for diving into this and giving a great concrete example. Please let me know if there's something incorrect or missing. 💙

@SchnWalter
Copy link
Contributor Author

SchnWalter commented Mar 12, 2020

I see your point. Thanks for the update and the feature.

I've created a followup MR to fix some parts of ng add schematic that were still hardcoded to use app as project name: ionic-team/ionic-framework#20768

P.S. I'll create a followup MR to port some tests and fixes from the old MR: #4311

@imhoffd
Copy link
Contributor

imhoffd commented Mar 12, 2020

Thanks. Apologies it took so long for me to look at it.

That all sounds good! I look forward to your PR.

@SchnWalter
Copy link
Contributor Author

Added MR #4349

@imhoffd
Copy link
Contributor

imhoffd commented Mar 14, 2020

Awesome! I'm happy these commands are now up to snuff. Cheers 🍻

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