Add documented public api

[nicojs]

Fixes #8744

@filipesilva here it is. I tried to follow the guidance in your latest remark in #8744. Could you take a look.

Is type information shipped with the angular cli? Would that be possible? Having type information for the programmatic use case would be nice.

[filipesilva]

I think this is the right direction, yes. Added some small requests. Tests are also failing because of the changed API though.

The CLI will ship with type information, yes. You should get full typings when you import it.

[filipesilva]

cli/lib/index.ts can be renamed to main.ts instead, no need for an extra file.

[nicojs]

Ok, but than it will be exported with a default export. That is OK?

[filipesilva]

I think the default export is ok, yes.

[filipesilva]

Can you change cliArgs to just args? Being a public api the name is now more important.

[nicojs]

@filipesilva

I've implemented your review comments. The api change is back (default export), should be green now.

[filipesilva]

I'm seeing red CI, I think it's because of this error:

TS2307: Cannot find module '@angular/cli/lib/cli'.

10 import cli from '@angular/cli/lib/cli';

Likely due to the filename change.

We also validade commit messages (https://github.com/angular/angular-cli/blob/master/CONTRIBUTING.md#-commit-message-guidelines). A think a good single commit name would be feat(@angular/cli): add public programmatic API.

In regards to when you can expect it, it would be 7.1 at the earliest. We are in a RC period already which means no no features are merged. But 7.1 would be shortly after 7.0.

[nicojs]

Both issues should be fixed now.

In regards to when you can expect it, it would be 7.1 at the earliest. We are in a RC period already which means no no features are merged. But 7.1 would be shortly after 7.0.

That's fine. The API was already there in hindsight. I didn't realize that. This PR is really only documenting the hidden feature.

[nicojs]

@filipesilva any idea what's going wrong with the build? Sorry for the mess. 🗑

[filipesilva]

I think one of our build error reporting functions isn't reporting the error correctly.

But it is in https://circleci.com/gh/angular/angular-cli/21583:

TS2345: Argument of type '{ cliArgs: string[]; }' is not assignable to parameter of type 'CliOptions'.
    Object literal may only specify known properties, and 'cliArgs' does not exist in type 'CliOptions'.

So somewhere the cliArgs is still being passed. You should be able to see this locally by running npm run build. It will not exit with an error code but should show it.

[nicojs]

I seem to have a problem with installing the dependencies:

I'll try negotiating with the build server 🙈

[nicojs]

This seems to be better. Build is green 👍

[filipesilva]

LGTM 👍

[filipesilva]

Adding a change request to block it for 7.1, and to make sure the change is brought up in a meeting.

[nicojs]

Great, thanks. This makes for a stable Stryker integration 👍

[filipesilva]

Thinking about the whole args vs cliArgs issue makes me think we should keep cliArgs... As you said, the API had exposed for a while now, it just wasn't documented. That means some people might be relying on it.

WDYT of changing it back to cliArgs, keep the internal rename changes, and renaming the commit to docs(@angular/cli): document public programmatic API?

That better reflects the work that ended up being done in this PR, and doesn't require a minor. We could get it in in 7.0.

[nicojs]

Hmm good point. Didn't think of that, this will break Stryker as well...

However I do agree that args might be better. We could also deprecate cliArgs, but make sure it still works. If both args and cliArgs are used, cliArgs are ignored.

However, changing it back to cliArgs is fine by me to.

[filipesilva]

That's a ways to go about it. I'll bring it up with the team and see what they prefer as well.

[nicojs]

If you're worried about backward compatibility, we could also leave the name index.ts instead of main.ts, or keep an index.ts with a deprecated message and proxy to main.ts

[filipesilva]

That part doesn't bother me much, as it's not part of the public API per se. Importing @angular/cli exposes the API, but importing @angular/cli/lib/index is a deep import and not part of the API.

[filipesilva]

@nicojs we discussed this within the team and decided to not document this API right now. Documenting it implies stability, and we can't commit to this API being stable yet. One of the changes we're planning is to separate local and global CLI and that will most likely affect his API.

I think not having this PR merged won't affect your usecase because you are already using the existing API and it hasn't changed. But let me know if it does affect you.

[nicojs]

Documenting it implies stability, and we can't commit to this API being stable yet.

Well.. strictly speaking it would force you to not change the API with a patch/minor release, which helps Stryker and other tools (intellij plugin, etc).

We are indeed not effected right now. However, could you try to do the following:

• Don't break this undocumented API with a patch/minor release
• When you break it with a major release, add it to the changelog (even though it is undocumented). Preferably with an example of the new API

That would be great.

[filipesilva]

If/when we change it, I expect it to be well documented. I can't 100% guarantee it will be in a major, but can say that we avoid changing that sort of things in patch/minor especially now that we're all aware there are at least a couple of tools that are using it.

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}