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

Create Angular package exposing ApiEditorComponent #316

Merged
merged 6 commits into from
Jun 7, 2018

Conversation

zregvart
Copy link

@zregvart zregvart commented Jun 7, 2018

In https://github.com/syndesisio/syndesis we're considering embedding a OpenAPI editor functionality of Apicurio. To do this we would like to depend on a Angular module that contains only the ApiEditorComponent.

This is the first step in creating an Angular package that only exposes the ApiEditorComponent. More details are provided in the individual commit messages.

The base commit that accomplishes the creation of the package and exposes the ApiEditorComponent is in 8402911. This is done by using https://github.com/dherges/ng-packagr/. Other commits resolve errors/warnings emitted from the ahead of time compilation.

Next steps might be to use this ApicurioEditorModule internally within Apicurio so that it's always up to date with the dependencies or other code changes to the ApiEditorComponent, or to create a test that embeds the ApiEditorComponent from the ApicurioEditorModule to validate the embedding scenario.

This adds `public_api.ts` file that exports the `ApiEditorComponent`
along with all the dependencies. To create the package invoke
`yarn package` that uses ng-packagr to help with the ahead of time
compilation and bundling. The resulting Angular package is created in
the root directory of the project as `dist.tgz`.
Explicitly importing specific RxJS operators helps to make the bundle
size smaller and helps with ahead of time compiling.
`filters` property doesn't seem to be referenced outside of the
ApisPageComponent it seems to make sense to make it `private`.
This helps the ahead of time compilation.
This resolves the warning reported while performing ahead of time
compilation of the public module.

```
warning TS0: the type annotation on @param is redundant with its TypeScript type, remove the {...} part
```
...injection

To help dependency injection when the code is ahead of time compiled
constructors need to be explicitly declared.
@EricWittmann
Copy link
Member

Thanks! I'll get this merged today.

Copy link
Member

@EricWittmann EricWittmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have some inline questions that you may have answered already in your commit messages. I'll go check that now. :)

},
"private": true,
"dependencies": {
"peerDependencies": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain the changes in package.json and how they will impact apicurio when built as an application (vs. as a module to be used in e.g. syndesis)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is my understanding of dependencies vs peerDependencies: when we use dependencies NPM (Yarn) will put the dependencies listed within, whereas with peerDependencies it will put them on the same level with Apicurio.

The difference can be seen in node_modules when you add Apicurio as a dependency:

Declared as dependencies:

node_modules
├── apicurio-design-studio
...
│   ├── node_modules
│   │   ├── bootstrap-datepicker
...
│   │   │   ├── node_modules
│   │   │   │   └── jquery
...
│   │   ├── bootstrap-switch
...
│   │   ├── brace
│   │   │   └── index.d.ts
│   │   ├── camel-case
│   │   │   └── camel-case.d.ts
...

Declared as peerDependencies:

├── apicurio-design-studio
...
│   ├── node_modules
│   │   ├── brace
│   │   │   └── index.d.ts
│   │   ├── camel-case
│   │   │   └── camel-case.d.ts
...

Notice that we then don't get bootstrap-datepicker or bootstrap-switch (and so on) dependencies in the project that uses Apicurio.

If you try to declare dependencies as dependencies, you get an error from ng-packagr:

Distributing npm packages with 'dependencies' is not recommended. Please consider adding @angular/animations to 'peerDependencies' or remove it from 'dependencies'.

BUILD ERROR
Dependency @angular/animations must be explicitly whitelisted.
Error: Dependency @angular/animations must be explicitly whitelisted.

This is somewhat similar to optional dependencies when using Maven.

@@ -20,7 +20,7 @@ import {ModalDirective} from "ngx-bootstrap";
import {OasDocument, OasLibraryUtils} from "oai-ts-core";
import {FormControl} from "@angular/forms";
import {Subject} from "rxjs/Subject";

import 'rxjs/add/operator/distinctUntilChanged';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this added? I can't see where it is used...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see - I have the rxjs operators imported globally in the application, but it's needed here as well when using as a component of syndesis. (I presume) :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this prevents bundling all of RxJS, a better option might be to have a RxJS include file, and list only the modules that are used. Bundling the whole RxJS (as in app.module.ts) tends to increase the bundle size.

I think this rationale is explained well in this blog post.

@@ -261,7 +261,7 @@ export abstract class TextAreaEditorComponent extends TextInputEditorComponent {

/**
* Subclasses can override this to provide validation status of the current value.
* @return {boolean}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of jsdoc changes in this diff. I think WebStorm adds this automatically. I'm happy to remove them all but am wondering why. :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems that ng-packagr is treating warnings as errors, so to help with this warning:

warning TS0: the type annotation on @param is redundant with its TypeScript type, remove the {...} part

I made that change so that the packaging wouldn't fail.

See ng-packagr/ng-packagr#413

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah figured that one out. WebStorm adds the {Type} automatically ATM, but the next patched version of it will add an option to toggle that behavior (and set the default for typescript to false). So that's good. :)

I'll go through all of Apicurio at some point and remove them.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could do find . -name '*.ts' -exec sed -i -e 's/\(@param\|@return\) {[^}]*}/\1/' {} \; or some other bulk magic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 - I do that sort of thing with regular expressions from within Eclipse. It has nice support for find/replace across files using regexp and with full preview before approving the change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -78,7 +78,7 @@ export class ApisPageComponent extends AbstractPageComponent implements OnDestro
public allApis: Api[] = [];
public filteredApis: Api[];
public selectedApis: Api[];
public filters: Filters = new Filters();
private filters: Filters = new Filters();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason to make this particular field private? I'm sure there are lots of fields around that could be private but are not.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't remember exactly, but I think the ahead of time compilation failed as I haven't imported Filters in the public_api.ts, being that Filters is only used here made more sense to make it private.

@EricWittmann EricWittmann merged commit 23e79a2 into Apicurio:master Jun 7, 2018
@zregvart zregvart deleted the angular-package branch June 8, 2018 07:27
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.

None yet

2 participants