feat: support multiple ionic projects #3297

stupidawesome · 2018-06-09T15:19:06Z

This PR adds experimental support for multiple Ionic apps within a shared workspace by allowing separate cordova integrations to be configured for each project. This is achieved by augmenting the ionic.config.json object with new properties in a way that is backwards compatible with existing single app projects. This is currently limited to angular project types with cordova integrations only.

Related issue: #3281

Tests should be passing but I need to write some more that cover multi-app scenarios.

…. Related ionic-team#3281 Adding `--project` support for almost every Ionic command. Create an `ionic.json` config in project root. This closely mirrors `angular.json` but doesn't depend on it. ``` { "defaultProject": "app", "projects": { "app": { "name": "Ionic App", "type": "angular", "integrations": { "cordova": { "root": "cordova" } }, "hooks": {} } } } ``` Example commands ``` # With project name ionic cordova platform add android --project app # With defaultProject ionic cordova platform add android # Other commands ionic cordova resources --project app ionic cordova plugin add cordova-plugin-splashscreen --project app ionic cordova build --project app ionic cordova run --project app ionic cordova emulate --project app ionic cordova requirements --project app ionic cordova prepare --project app ```

By treating each "project" as its own self-contained ionic configuration within "projects" we can reuse most of the existing command logic.

imhoffd · 2018-06-10T18:39:53Z

packages/@ionic/cli-framework/src/utils/fs.ts

+/**
+ * Find the base directory based on the path given and a marker file to look for.
+ */
+export async function findProjectDirectory(dir: string, file: string): Promise<string | undefined> {


Doesn't look like this is used.

imhoffd · 2018-06-10T18:53:45Z

packages/@ionic/cli-utils/src/definitions.ts

@@ -98,6 +99,15 @@ export interface ProjectFile {
    key?: string;
    cert?: string;
  };
+
+  type: ProjectType;


If I remember correctly, I took this property out because I wanted to remember to use env.project.type, not projectConfig.type. The project configuration is kind of bad and annoying, sorry. I've been meaning to refactor it.

imhoffd · 2018-06-10T19:27:16Z

packages/@ionic/cli-utils/src/lib/config.ts

  protected configFile?: T;
  protected originalConfigFile?: { [key: string]: any };

-  constructor(directory: string, public fileName: string) {
+  constructor(directory: string, readonly fileName: string, readonly name: string | undefined) {


Why is name needed in BaseConfig? It should only relate to Project, right?

imhoffd · 2018-06-10T20:07:46Z

packages/@ionic/cli-utils/src/definitions.ts

+export interface MultiProjectConfig {
+  defaultProject: string;
+  projects: {
+    [key: string]: ProjectConfig;


Should probably be [key: string]: ProjectConfig | undefined;

I added the type guards isProjectConfig() and isMultiProjectConfig() to check if the projects key exists and toggle behaviour accordingly.

But still if I access projectConfig.projects.asndgksjehbnrgfvkljahsefdkjghasekjhfgakjhsdg then TypeScript will say it's a ProjectConfig, when it possibly is not there at all.

imhoffd · 2018-06-10T20:12:07Z

packages/@ionic/cli-utils/src/lib/project/index.ts

+    }
+
+    return {
+      enabled: integration.enabled === undefined ? false : integration.enabled,


Integrations are only disabled when the enabled key exists and is false, so this should be:

integration.enabled === undefined ? true : integration.enabled,

or

integration.enabled !== false,

imhoffd · 2018-06-10T20:54:27Z

packages/@ionic/cli-utils/src/lib/build.ts

@@ -146,6 +165,11 @@ class BuildAfterHook extends Hook {
 export async function build(env: IonicEnvironment, inputs: CommandLineInputs, options: CommandLineOptions): Promise<void> {
  try {
    const runner = await BuildRunner.createFromProject(env, env.project);
+
+    if (env.project.name) {
+      options['project'] = env.project.name;


I was trying to think of a better spot for this, but I think this is it 😂

imhoffd · 2018-06-10T21:06:12Z

packages/@ionic/cli-utils/src/lib/project/index.ts

    }

    for (const projectType of PROJECT_TYPES) {
-      const p = await Project.createFromProjectType(projectDir, PROJECT_FILE, deps, projectType);


Project type detection doesn't work for a multi-app config. Here's the config I tried:

{ "defaultProject": "sidemenu", "projects": { "sidemenu": { "root": "packages/sidemenu" }, "tabs": { "root": "packages/tabs" } } }

Try this:

ionic.config.json

{ "defaultProject": "sidemenu", "projects": { "sidemenu": { "type": "angular" }, "tabs": { "type": "angular" } } }

angular.json

{ "defaultProject": "sidemenu", "projects": { "sidemenu": { "root": "packages/sidemenu" }, "tabs": { "root": "packages/tabs" } } }

Sorry, I was just trying to convey that determineType() doesn't work for multi-app projects. I'm thinking the function needs to be refactored to either work or throw a fatal error.

Remove the type attributes from ionic.config.json to see what I mean.

Should this perhaps throw an error instead?

https://github.com/ionic-team/ionic-cli/blob/master/packages/%40ionic/cli-utils/src/lib/project/index.ts#L78

Edit: I see what you mean.

I added more exceptions so it should give more meaningful error messages now.

Traditionally determineType() wasn't meant to throw fatal errors. If a project type cannot be determined, it's actually fine, it just means we can't use project commands. The CLI switches to "global mode" if project type cannot be determined.

But if the behavior is changed, it's fine. We don't have integration tests for behavior, so it's mostly going to be me testing things and making tweaks. 😆

imhoffd · 2018-06-10T21:17:26Z

packages/@ionic/cli-utils/src/lib/build.ts

@@ -26,6 +39,11 @@ export const COMMON_BUILD_COMMAND_OPTIONS: ReadonlyArray<CommandMetadataOption>
    summary: `Target platform on chosen engine (e.g. ${['ios', 'android'].map(e => chalk.green(e)).join(', ')})`,
    groups: [OptionGroup.Advanced],
  },
+  {
+    name: 'project',
+    summary: 'The name of the project',


What's the reason to add this here? --project isn't a common build option, it's a global option for angular projects, right?

If the reason is to add it to the help output, we can do something else instead

Project needs to be a top level option to support ionic commands that are not related to angular:

Eg:

ionic cordova requirements --project name ionic cordova resources --project name ionic cordova platform add ios --project name ionic cordova plugin add cordova-plugin-splashscreen --project name

I also wanted to make sure these changes aren't tightly coupled to angular. The only hard requirement for angular projects right now is that a project in ionic.config.json maps to a project with the same name in angular.json.

With that said, I don't see --project appearing in these help dialogs, so I'm not sure I put this in the right place.

That all makes sense, I just don't think it should be listed here (or in the common serve options). It's okay to leave it for now.

I need to do some refactoring for these commands. Maybe have "runners" for each command/project type combination. Then it'd be easy to add the option to the project type(s) that support global options like this.

imhoffd · 2018-06-10T21:22:11Z

packages/ionic/src/commands/link.ts

        });
      }

-      proId = await this.createApp({ name }, runinfo);
+      if (name) {


Why was this necessary?

I think I was getting some TypeScript validation error, but I'm not seeing it now so I'll revert it.

imhoffd · 2018-06-10T21:23:23Z

packages/ionic/src/commands/start.ts

@@ -434,7 +434,7 @@ ${chalk.cyan('[1]')}: ${chalk.bold('https://ionicframework.com/docs/cli/starters
    // start is weird, once the project directory is created, it becomes a
    // "project" command and so we replace the `Project` instance that was
    // autogenerated when the CLI booted up. This has worked thus far?
-    this.env.project = await getProject(projectDir, this.env);
+    this.env.project = await getProject(projectDir, String(options['name']), this.env);


Probably better to pass undefined here instead? options['name'] refers to something else

Yeah I wasn't sure what to put here, is this after the project is created? Will the name entered in ionic start also be used as the project name for the app that gets generated? Or will they always default to app?

imhoffd · 2018-06-10T21:24:47Z

Overall this is great work. 💪 I can't wait to get this in!

kevin-lot · 2018-06-14T09:28:04Z

I am very happy to see this PR merged.
Are you planning to add the same spirit to capacitor client ?

(and how can we use the client with this new feature ?)

timbru31 · 2018-08-01T09:39:01Z

FYI this does not work with the Snapshot Build of Ionic Pro (yet). I've opened a support ticket to have this issue addressed.
Change is awesome nevertheless! 👍

imhoffd · 2018-08-01T16:00:46Z

@kevin-lot Yes, this will work for Capacitor.

@timbru31 Thanks for the heads up. I've been working with @nphyatt to get this feature working in Ionic Pro by using the Ionic CLI for builds. Note that this will only be for Ionic 4 beta, so some rough spots are to be expected. 😊

edcarroll · 2018-09-02T13:15:30Z

I've been working with the new multi project setup and it's been fantastic, though have run into an issue: when running any Cordova commands on Windows, the use of an absolute path in --cordova-base-path stops the Angular CLI from actually outputting the built code in the correct place, which then makes cordova prepare fail as the www folder is missing.

Using a relative path fixes this (I believe it's when the absolute path is normalized, which converts C:\ to /C/) - though I've not yet worked out exactly where this absolute path is coming from. Hoping to open a PR when I find it, but if anyone spots this and knows where to go that would be very helpful!

imhoffd · 2018-09-03T17:57:24Z

@edcarroll Interesting. I wonder if the Angular Devkit has issues with using an absolute path within a workspace. There are levels of abstraction with paths and it may get confused on Windows.

If you're willing to look into it (I would love a PR ❤️), check out these points in the code base:

Ionic CLI provides --cordova-base-path option here:

ionic-cli/packages/@ionic/cli-utils/src/lib/project/angular/build.ts

Line 109 in 2862762

args.cordovaBasePath = integration.root;

ng-toolkit (using Angular Devkit) picks up the option and normalizes the path here:

ionic-cli/packages/@ionic/ng-toolkit/src/cordova-build/index.ts

Line 37 in 49d9493

    
           const cordovaBasePath = normalize(options.cordovaBasePath ? options.cordovaBasePath : '.');

(also look for the same option in the serve code for Cordova livereload as well)

Maybe the solution is to use a relative path, because it's likely what the Angular Devkit expects within a workspace.

stupidawesome added 2 commits June 9, 2018 15:23

refactor: adapt multi app workspace to single app config

1b6d288

By treating each "project" as its own self-contained ionic configuration within "projects" we can reuse most of the existing command logic.

stupidawesome requested a review from imhoffd as a code owner June 9, 2018 15:19

fix: start and config commands

3317570

imhoffd reviewed Jun 10, 2018

View reviewed changes

stupidawesome and others added 8 commits June 11, 2018 11:52

refactor: clean up code and pass cwd to shell commands

2b75d57

fix: syntax and runtime errors

d2e467f

fix: throw exceptions when trying to use unknown project name

3154155

fix: throw exceptions more reliably when project type cannot be detected

f0c358e

feat(ng-toolkit): pass through cordova base path to ng cli

965dfce

test: add a few more tests

dfbe68b

Merge branch 'master' into support-multiple-ionic-projects

b99ac40

Merge branch 'master' into support-multiple-ionic-projects

09c1c70

imhoffd merged commit 2d53c99 into ionic-team:master Jun 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support multiple ionic projects #3297

feat: support multiple ionic projects #3297

stupidawesome commented Jun 9, 2018

imhoffd Jun 10, 2018

imhoffd Jun 10, 2018

imhoffd Jun 10, 2018

imhoffd Jun 10, 2018

stupidawesome Jun 11, 2018 •

edited

Loading

imhoffd Jun 11, 2018

imhoffd Jun 10, 2018

imhoffd Jun 10, 2018

imhoffd Jun 10, 2018

stupidawesome Jun 11, 2018

imhoffd Jun 11, 2018

stupidawesome Jun 11, 2018 •

edited

Loading

stupidawesome Jun 11, 2018

imhoffd Jun 11, 2018

imhoffd Jun 10, 2018

stupidawesome Jun 11, 2018

stupidawesome Jun 11, 2018 •

edited

Loading

imhoffd Jun 11, 2018

imhoffd Jun 11, 2018

imhoffd Jun 10, 2018

stupidawesome Jun 11, 2018

imhoffd Jun 10, 2018

stupidawesome Jun 11, 2018

imhoffd commented Jun 10, 2018

kevin-lot commented Jun 14, 2018

timbru31 commented Aug 1, 2018

imhoffd commented Aug 1, 2018

edcarroll commented Sep 2, 2018

imhoffd commented Sep 3, 2018

feat: support multiple ionic projects #3297

feat: support multiple ionic projects #3297

Conversation

stupidawesome commented Jun 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stupidawesome Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stupidawesome Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stupidawesome Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imhoffd commented Jun 10, 2018

kevin-lot commented Jun 14, 2018

timbru31 commented Aug 1, 2018

imhoffd commented Aug 1, 2018

edcarroll commented Sep 2, 2018

imhoffd commented Sep 3, 2018

stupidawesome Jun 11, 2018 •

edited

Loading

stupidawesome Jun 11, 2018 •

edited

Loading

stupidawesome Jun 11, 2018 •

edited

Loading