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

[typescript-angular] AOT-compatible API client (via ng-packagr and ngc) #7984

Merged
merged 9 commits into from
Apr 16, 2018
Merged

[typescript-angular] AOT-compatible API client (via ng-packagr and ngc) #7984

merged 9 commits into from
Apr 16, 2018

Conversation

JohannesHoppe
Copy link
Contributor

@JohannesHoppe JohannesHoppe commented Apr 6, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change.
  • Filed the PR against the correct branch.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language. @macjohnny

Description of the PR

This PR is a follow-up of PR #6735. It fixes #6722.
This solution simplifies the compilation drastically, by leveraging ng-packagr.
ng-packagr creates a complete new angular-package into dist - which can be easily published via npm publish dist (see generated README.md)

Depending on the angular version three different styles are generated:

To stress this a little bit, I tested a package with Angular 6.0.0-beta.5 and it works, too.

FYI
@taxpon @kenisteward @TiFu @Vrolijkx @sebastianhaas @macjohnny

TODO
We might want to fiddle around with the different TypeScript versions, each Angular Compiler has own requirements to the allowed range of TypeScript version. Right now compilation gives us a warnings, but the generated bundles are still fine.

@JohannesHoppe
Copy link
Contributor Author

During my journey I also wrote everything down.
You can read my raw (and unpublished) article here: Generate Angular API clients with Swagger
Feedback and corrections are very much appreciated! 😄:thumbsup: --> fork it on github

@kenisteward
Copy link
Contributor

We definitely need to mess with proper ts version generation as different versions of ng have different ts compatibility bases. This is great work and highly appreciated.

@JohannesHoppe
Copy link
Contributor Author

@kenisteward I agree. But as far as I know this only leets to a nasty warning. After everything is bundled, the final webpack build of the consuming project does not care about typescript at all. It's "just" a devDependency of the angular-lib.

@kenisteward
Copy link
Contributor

kenisteward commented Apr 6, 2018 via email

@JohannesHoppe
Copy link
Contributor Author

Honestly, a lot of black-magic is done here! When using ng-packagr 1 it somehow creates those bundles for angular 4. I confirm that everything is nicely compilable via npm run build. Don't ask my why it works! 😉

@kenisteward
Copy link
Contributor

kenisteward commented Apr 6, 2018 via email

and i feel generally better when `npm run build` is called explicitly.

- for ng-packagr 2 is doesn't matter, since the final package.json does not have any scripts
- for old ng-packagr 1 it matters, scripts are copied to the final package.json which breaks installation via `npm install {{npmName}} --save` (it runs `npm run build` again)
Copy link
Contributor

@macjohnny macjohnny left a comment

Choose a reason for hiding this comment

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

thanks for this PR. looks good

additionalProperties.put(NG_VERSION, ngVersion);
additionalProperties.put("injectionToken", ngVersion.atLeast("4.0.0") ? "InjectionToken" : "OpaqueToken");
additionalProperties.put("injectionTokenTyped", ngVersion.atLeast("4.0.0"));
additionalProperties.put("useHttpClient", ngVersion.atLeast("4.3.0"));
if (!ngVersion.atLeast("4.3.0")) {
supportingFiles.add(new SupportingFile("rxjs-operators.mustache", getIndexDirectory(), "rxjs-operators.ts"));
}

// the current text makes only sence when using it as an npm/angular package
supportingFiles.add(new SupportingFile("README.mustache", getIndexDirectory(), "README.md"));
Copy link
Contributor

Choose a reason for hiding this comment

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

the current text makes only sence when using it as an npm/angular package

yes, that's true, but I would still generate it also for non-npm packages, because this README should be extended to be a Readme on how to use the generated code, too.
E.g. in #7965 information is added on how to set the base path with an observable.

So I would revert this change and generate the README in any case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I will change that.


```
npm install PATH_TO_GENERATED_PACKAGE --save
npm install PATH_TO_GENERATED_PACKAGE/dist --save
```

_using `npm link`:_
Copy link
Contributor

Choose a reason for hiding this comment

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

add a note about an issue in angular CLI that prevents using linked libraries on Windows, see angular/angular-cli#8284

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@macjohnny
Copy link
Contributor

macjohnny commented Apr 6, 2018

@JohannesHoppe your article is excellent! you should add it as a Readme somewhere. @wing328 is there a place to publish language-specific help? wiki?

@macjohnny
Copy link
Contributor

@JohannesHoppe just wanted to point out that the newest angular cli version supports generating libraries: https://github.com/angular/angular-cli/releases/tag/v6.0.0-rc.2

maybe this could be useful here?

@JohannesHoppe
Copy link
Contributor Author

JohannesHoppe commented Apr 9, 2018

Yes, and no. 😄

Yes:

  • This would be another option to generate a angular-library in the "angular package format"
  • most people are familiar with the cli

No:

  • we would need to follow the angular-cli file structure, which means even more boilerplate files
  • I'm not sure if this will work in older Angular versions (Angular 2)
  • ng-packagr == rollup, angular-cli == webpack. it is said that rollup is much more faster
  • the RCs of angular-cli are super-unstable.

Generally, we have a lot of different petstore samples, which shows the amount of complexity. I would vote to skip Angular 2 and Angular 4.0-4.2 and to skip the "no npm package" part sooner or later. (But not in this PR!)

@macjohnny
Copy link
Contributor

@JohannesHoppe ok I agree, ng-packagr can be used with only one configuration file and without any additional changes, which is nice.

I would also agree to skip angular < 4.3

@JohannesHoppe
Copy link
Contributor Author

@macjohnny I added both improvements from your review. We are ready to merge, aren't we? :shipit:

@macjohnny
Copy link
Contributor

@JohannesHoppe thanks for the changes.
I think you need to re-generate the samples, as you changed the Readme.md
But apart from that, the PR seems fine from my point of view.

@JohannesHoppe
Copy link
Contributor Author

I was hoping that nobody will notice that... 😎 I will do it after work...

@JohannesHoppe
Copy link
Contributor Author

FYI:

ng g library <name>
It generates a new folder inside an Angular CLI project, ready to be built with ng-packagr.

https://twitter.com/juristr/status/984501406660022272

@kenisteward
Copy link
Contributor

kenisteward commented Apr 12, 2018 via email

@JohannesHoppe
Copy link
Contributor Author

JohannesHoppe commented Apr 12, 2018

This means that we won't go for angular-cli as @macjohnny suggested. Because we would have this pipeline

  1. you pull sagger-codegen and compile with mnv clean package
  2. swagger-codegen generates an angular-cli project
  3. ng g library generates ng-packagr project
  4. ng-packagr compiles a NPM package in "Angular package format"

With my PR we have:

  1. you pull sagger-codegen and compile with mnv clean package
  2. swagger-codegen generates ng-packagr project
  3. ng-packagr compiles a NPM package in "Angular package format"

... people will think we are completely crazy doing 4 steps! 😆

@kenisteward
Copy link
Contributor

kenisteward commented Apr 13, 2018 via email

@wing328
Copy link
Contributor

wing328 commented Apr 13, 2018

If no further feedback/question on this PR, I'll merge it into master on the coming weekend.

@damianmigo
Copy link

Did someone have to copy the metadata.json to the dist folder manually before publishing it?

I can’t build the project that depends on my client library with AOT because the decorators are missing.

@hypery2k
Copy link

I've tried to use it with Angular 8 and AoT enabled. For this some change are need see here

Any feedback is welcome

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

Successfully merging this pull request may close these issues.

[typescript-angular] generated package/ApiModule is not AOT-compatible
6 participants