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

Angular 8 and Angular CLI build infrastructure #622

Merged
merged 11 commits into from
Feb 7, 2020

Conversation

avatsaev
Copy link
Contributor

@avatsaev avatsaev commented Oct 4, 2019

Changes

This MR introduces the correct way to build modern Angular Libraries by using the Angular CLI build infrastructure.
It also ensures the compatibility with future Angular versions as well as Angular Ivy compatibility.

  • projects/angular-jwt/ folder contains the library coude source
  • src folder contains a mock app for the the library
  • unit testing happens in the mock app (src/app/services/example-http.service.spec.ts)

Running tests: npm run ng test

There aren't any major changes in the library source code, or any breaking changes.

The compiled library was tested on a large internal project, all unit tests passed, the application compiled to ivy without any errors, and authentication was working correctly wihtout any changes to the host application source code.

References

Checklist

Copy link

@stevehobbsdev stevehobbsdev left a comment

Choose a reason for hiding this comment

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

Hey @avatsaev

Thanks for the PR! It would be great to get this in.

A couple of things right off the bat before I look into this further:

  • Can you remove the .idea folder?
  • Can you restore the readme file, with any additional changes necessary that relate to this PR? Your changes have wiped out all of the installation/usage instructions, and we'd like to keep these

Thanks!

@avatsaev
Copy link
Contributor Author

avatsaev commented Oct 4, 2019

Will do!

@avatsaev
Copy link
Contributor Author

avatsaev commented Oct 4, 2019

Done, sorry about the readme file, missed it while copying, the instructions are the same, there is nothing to change.

@avatsaev avatsaev requested a review from stevehobbsdev October 4, 2019 09:49
@stevehobbsdev stevehobbsdev added the large Large review label Oct 4, 2019
@avatsaev
Copy link
Contributor Author

@stevehobbsdev if you have any questions please let me know, will be happy to help

@stevehobbsdev
Copy link

Will do @avatsaev. It needs a discussion internally about how we go about this - thanks for your patience.

@Neetesh1
Copy link

@avatsaev could you please tell, when you are merging this PR?

@avatsaev
Copy link
Contributor Author

@Neetesh1 once the maintainers approve it, it's not up to me

@aecz
Copy link

aecz commented Dec 2, 2019

@avatsaev Could you review the conflicts ? Also I think you should mark the conversation as resolved where @stevehobbsdev requested you made changes. After that, this PR will have all checks green and be ready to merge. Thanks!

@avatsaev
Copy link
Contributor Author

avatsaev commented Dec 3, 2019

@aecz I can't mark anything as resolved here, I just pushed the requested changes and asked for review again. Still waiting

@aecz
Copy link

aecz commented Dec 3, 2019

@avatsaev Can you solve the branch conflicts at least ?

@avatsaev avatsaev requested a review from a team December 3, 2019 19:10
@avatsaev
Copy link
Contributor Author

avatsaev commented Dec 3, 2019

resolved.

@s992
Copy link

s992 commented Dec 11, 2019

@stevehobbsdev Is there anything we can do to help get this across the line? This package is currently blocking our upgrade to Ivy, so would love to assist in any way we can.

@avatsaev
Copy link
Contributor Author

Hey @s992 we couldn't wait for this to get reviewed soon enough so we deployed this version of the PR on npm in the meanwhile, been using it for a month or so, works very well, it's under the name of https://www.npmjs.com/package/@avatsaev/angular-jwt

I know it's not the best solution, but we had to move forward, if you're not comfortable using the npm package you can clone my repo, checkout the angular-cli-refactor branch, build and deploy the package on your private npm reg while we wait for this to be merged.

pinging @stevehobbsdev

@s992
Copy link

s992 commented Dec 11, 2019

Thanks @avatsaev !

@joshcanhelp joshcanhelp removed the request for review from a team January 27, 2020 21:30
@avatsaev avatsaev closed this Feb 7, 2020
@avatsaev avatsaev deleted the angular-cli-refactor branch February 7, 2020 14:11
@avatsaev avatsaev restored the angular-cli-refactor branch February 7, 2020 16:49
@avatsaev avatsaev reopened this Feb 7, 2020
@stevehobbsdev
Copy link

Thanks for raising this again @avatsaev, and I do apologise for the mishap here.

We can bring your changes in and release as a new major version. However, to do that I will need you to undo your changes that switch out the @auth0 namespace and remove the Auth0 info on the readme, plus anything else of that nature that is present in the PR.

Let me know when that is done 👍

@avatsaev
Copy link
Contributor Author

avatsaev commented Feb 7, 2020

@stevehobbsdev done, documents restored, I don't think i've missed anything

@stevehobbsdev stevehobbsdev linked an issue Feb 7, 2020 that may be closed by this pull request
Copy link

@stevehobbsdev stevehobbsdev left a comment

Choose a reason for hiding this comment

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

Thank you!

@stevehobbsdev stevehobbsdev merged commit 20c723e into auth0:master Feb 7, 2020
@mohyeid
Copy link

mohyeid commented Feb 7, 2020

This is great, but there is 49 files changed. Can you summarize what changed? Or any reference what do we need to change to make our lib work? We are facing the same issue

@stevehobbsdev
Copy link

stevehobbsdev commented Feb 7, 2020

@avatsaev is best placed to provide that summary. The interface should remain the same when you're actually using the library.

@avatsaev
Copy link
Contributor Author

avatsaev commented Feb 7, 2020

The lib sources themselves haven't changed (except rxjs v6+ pipes refactor), it's now just wrapped around Angular CLI builder which makes the compiled lib compatible with Angular 9. So the host apps don't need to change anything, just build the lib and ship the v4 and it should be fine.

@stevehobbsdev stevehobbsdev mentioned this pull request Feb 7, 2020
@stevehobbsdev
Copy link

This has now been published as v4, thanks again @avatsaev and apologies for the delay in getting this out.

@ltomes
Copy link

ltomes commented Feb 17, 2020

Hi, is any additional configuration required to use 4.0.0?
I'm running into the issues described on #605 (comment) with version 4.0.0 when trying to use the JwtHelperService (Before 4.0.0 I can't get Ivy to compile anyway).

Thanks

@stevehobbsdev
Copy link

@ltomes There should be no need for any additional configuration, no. With version 4, you should just be able to do import { JwtHelperService } from '@auth0/angular-jwt' and have it work. You would have no need for the workaround described in that thread (and like you say, the src directory is no longer included in the build).

If you have any specific error messages and/or a small project that reproduces the problem, I'd be happy to take a look.

@ltomes
Copy link

ltomes commented Feb 18, 2020

Hi Steve, thanks for confirming no other changes are required.
My issue appears unrelated to the angular-jwt update. My application is calling a different npm module (openapi-generator-cli generated code) after the helper service that fails to properly AOT compile (But gives no errors/warnings at compile time) in ivy mode.

The joys of updating multiple dependencies at once!

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

Successfully merging this pull request may close these issues.

You are using an outdated rxjs import
7 participants