-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Conversation
- uses ngc when targeting Angular 2 (as seen in #6735) - uses ng-packagr 1 when targeting Angular 4 - uses ng-packagr 2 when targeting Angular 5
During my journey I also wrote everything down. |
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. |
This reverts commit 471d248.
@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. |
For using ngc with 2.7 ts and ng 4 ( I think) aot compilation doesn't work
at all from when I was doing it by hand. So we should be careful to make
sure all of our settings generate proper libs is all.
…On Fri, Apr 6, 2018, 9:34 AM Johannes Hoppe ***@***.***> wrote:
@kenisteward <https://github.com/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.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7984 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AMPLtZd0cs5cC6zfI8O99y1UZZqC7e0hks5tl3zhgaJpZM4TKH2M>
.
|
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 |
Interesting!
…On Fri, Apr 6, 2018, 9:48 AM Johannes Hoppe ***@***.***> wrote:
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! 😉
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7984 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AMPLtSSaQfPFjHy-cwgzZ03HkgXeTKo4ks5tl4A0gaJpZM4TKH2M>
.
|
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)
There was a problem hiding this 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")); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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`:_ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅
@JohannesHoppe your article is excellent! you should add it as a Readme somewhere. @wing328 is there a place to publish language-specific help? wiki? |
@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? |
Yes, and no. 😄 Yes:
No:
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!) |
@JohannesHoppe ok I agree, I would also agree to skip angular |
@macjohnny I added both improvements from your review. We are ready to merge, aren't we? |
@JohannesHoppe thanks for the changes. |
I was hoping that nobody will notice that... 😎 I will do it after work... |
FYI:
|
Doesn't that requires us to refactor to make sure we fully confirm to the
generated package? Or do we already confirm?
I love me some angular cli haha. It makes our ut functional easier to
generate as well.
…On Thu, Apr 12, 2018, 4:29 PM Johannes Hoppe ***@***.***> wrote:
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
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7984 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AMPLtVh4Xc4NKefBatF2RdA-dLai9Tqgks5tn7k_gaJpZM4TKH2M>
.
|
This means that we won't go for angular-cli as @macjohnny suggested. Because we would have this pipeline
With my PR we have:
... people will think we are completely crazy doing 4 steps! 😆 |
Hahaha
…On Thu, Apr 12, 2018, 6:00 PM Johannes Hoppe ***@***.***> wrote:
This means that the wo won't go for angular-cli as @macjohnny
<https://github.com/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 project compiles 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 project compiles NPM package in "Angular package format"
... people will think we are completely crazy doing having 4 steps! 😆
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7984 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AMPLtReTUXD4CbG314Cv7WZgOln81koLks5tn85qgaJpZM4TKH2M>
.
|
If no further feedback/question on this PR, I'll merge it into master on the coming weekend. |
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. |
I've tried to use it with Angular 8 and AoT enabled. For this some change are need see here Any feedback is welcome |
PR checklist
./bin/
to update Petstore sample so that CIs can verify the change.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 vianpm publish dist
(see generated README.md)Depending on the angular version three different styles are generated:
ngc
when targeting Angular 2 (as seen in Initial step towards an AOT-compatible API client. #6735)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.