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

feat(cli): Allow to use capacitor commands in custom platforms #3091

Merged
merged 19 commits into from
Jul 1, 2020

Conversation

jcesarmobile
Copy link
Member

@jcesarmobile jcesarmobile commented Jun 11, 2020

This is a POC

It allows to have custom platforms in Capacitor

Example platform https://github.com/jcesarmobile/test-platform

Steps to test:

  1. Install custom platform: npm install https://github.com/jcesarmobile/test-platform
  2. Run capacitor CLI commands by passing the platform name: examples npx cap add @jcesarmobile/test-platform, npx cap copy @jcesarmobile/test-platform, npx cap update @jcesarmobile/test-platform, npx cap sync @jcesarmobile/test-platform, npx cap open @jcesarmobile/test-platform.

Custom platforms need 4 scripts in scripts section of package.json (not all mandatory if the platform don't need them, but will show an error)

"scripts": {
    "capacitor:add": "script to run on npx cap add",
    "capacitor:copy": "script to run on npx cap copy",
    "capacitor:update": "script to run on npx cap  update",
    "capacitor:open": "script to run on npx cap  open"
  }

sync runs copy + update

At first I also did a npx cap add customPlatorm command that just ran npm install on the custom platform (could be http url, npm package, local folder, etc. whatever npm install supports), but then thought it might not be a good idea as it install the package and might install the wrong thing if the user type it wrong, so I think it's better not to wrap it and just tell the user to use npm install

cli/src/tasks/copy.ts Outdated Show resolved Hide resolved
cli/src/tasks/sync.ts Outdated Show resolved Hide resolved
cli/src/tasks/update.ts Outdated Show resolved Hide resolved
@imhoffd
Copy link
Contributor

imhoffd commented Jun 16, 2020

I'm thinking we actually should implement add. Here are my thoughts:

npm install <pkg> to install it into node_modules
npx cap add <pkg> to add it the app

Then, when using sync/copy/update via npx cap <command> <platform>, it resolves <platform> based on these rules:

  1. @capacitor/<platform> (official platforms)
  2. @capacitor-community/<platform> (community platforms)
  3. <platform> (third-party)

Once the package is resolved, it runs the capacitor:* scripts in the package's package.json

@imhoffd
Copy link
Contributor

imhoffd commented Jun 23, 2020

@jcesarmobile @IT-MikeS please review

@imhoffd imhoffd linked an issue Jun 23, 2020 that may be closed by this pull request
@IT-MikeS
Copy link
Contributor

Haven't been able to dig a ton but this current state works for me in testing after this.initElectronConfig(); in setCurrentWorkingDir(....) {....} in config.ts of the CLI is commented out.

Only issue I found in the little time I got was that we lose the console output from the script that is run via capacitor:add hook

@imhoffd
Copy link
Contributor

imhoffd commented Jun 24, 2020

@IT-MikeS I noticed that, too. If you don't mind, I'd like to tackle that at a later time. It may require some CLI refactoring.

@IT-MikeS
Copy link
Contributor

IT-MikeS commented Jun 24, 2020

@dwieeb Yeah thats fine, it functions, and the logs do show after the script ends so if an error is thrown the user can see it and deal with it.

Just need to make sure error origins are clear in the scripts, but that's up to the custom platform 👍

@imhoffd imhoffd added this to the 2.3.0 milestone Jun 26, 2020
@IT-MikeS
Copy link
Contributor

This PR (jcesarmobile#1) uses spawn instead of exec to run commands and thus doesn't hide live console output.

@imhoffd
Copy link
Contributor

imhoffd commented Jun 26, 2020

To sum up, we want this feature to go into 2.3.0 so people can start using @IT-MikeS's new electron platform. We can't remove the existing electron platform without a breaking change (see #3140).

So, the caveat is that people will need to use the commands like this until Capacitor 3:

npx cap add @capacitor-community/electron
npx cap sync @capacitor-community/electron

Instead of simply using electron.

@IT-MikeS's PR is a quick fix to achieve more advanced output from the platform (we talked about it). For Capacitor 3, I would like to refactor how the Capacitor CLI handles subprocesses.

fix: support console output for platform hooks
@jcesarmobile
Copy link
Member Author

should we also add an open command?
people asked for it in the current electron platform and what it does is to run npm run electron:start, so maybe custom platforms can also have a custom open script

@imhoffd
Copy link
Contributor

imhoffd commented Jun 29, 2020

Yes. If platforms can't handle it, they can just print a message.

@IT-MikeS
Copy link
Contributor

I agree with adding it.

I'll put one in the community platform

cli/src/tasks/open.ts Outdated Show resolved Hide resolved
@jcesarmobile
Copy link
Member Author

I can't approve since the PR was originally mine, but looks good, updated my test platform and all commands work fine.

@imhoffd imhoffd merged commit 6aedfde into ionic-team:master Jul 1, 2020
@imhoffd imhoffd deleted the custom-platforms-poc branch July 1, 2020 20:27
imhoffd added a commit that referenced this pull request Jul 1, 2020
Co-authored-by: Dan Imhoff <dwieeb@gmail.com>
Co-authored-by: Mike S <13892112+Mike-Summ@users.noreply.github.com>
imhoffd added a commit that referenced this pull request Jul 1, 2020
Co-authored-by: Dan Imhoff <dwieeb@gmail.com>
Co-authored-by: IT-MikeS <20338451+IT-MikeS@users.noreply.github.com>
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.

Support community platforms
3 participants