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

Angular14 Upgrade #725

Closed
wants to merge 12 commits into from
Closed

Angular14 Upgrade #725

wants to merge 12 commits into from

Conversation

Rlcolli4
Copy link

@Rlcolli4 Rlcolli4 commented Jun 3, 2022

Changes

  • package.json packages were updated to be Angular 14 or higher versions
  • Other dependencies were updated to their latest versions for compatibility
  • Changed version number to 6.0.0
  • Added a pending version number and notes to the change log.

References

Checklist

@Rlcolli4 Rlcolli4 requested a review from a team as a code owner June 3, 2022 21:09
@igorosabel igorosabel mentioned this pull request Jun 6, 2022
Copy link

@demkalkov demkalkov left a comment

Choose a reason for hiding this comment

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

Please replace tslint with eslint

package.json Outdated Show resolved Hide resolved
angular.json Show resolved Hide resolved
@Rlcolli4
Copy link
Author

I'll fix this as soon as I can. I didn't remove it intentionally, I forked the master branch and did the upgrade. It's probably something I have locally that did it without my knowledge.

@Rlcolli4
Copy link
Author

Just migrated from tslint/codelyzer to eslint. Added lint back to places in the angular.json based on steps and documentation for converting.

@MumiaIrrequieta
Copy link

Can we ask @Sambego to review and merge this? "auth0/developer-relations-admin" link is dead so maybe no one is receiving the notifications and he's the latest user to commit to this project.
Thanks.

@juvegitau
Copy link

@Sambego are in a position to merge?

Copy link
Member

@frederikprijck frederikprijck 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 the PR. As mentioned in #712 , apologies about the radio silence here.

Could you also update the package-lock.json file to remove all the entries to npmjs.sitesusa.local?

CHANGELOG.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
angular.json Outdated Show resolved Hide resolved
@frederikprijck
Copy link
Member

Doing a fresh pull from this branch, removing the lock file followed by npm i results in an error.

The following changes should solve that, can you update the PR to incorporate these as well?

  • Set jasmine-core to ~3.8.0
  • Set karma-jasmine-html-reporter to ^1.7.0

Thanks!

@Rlcolli4
Copy link
Author

I updated the package.json and angular.json as requested! The two latest commits handled the reversion and then any other changes I needed to get npm start, npm run build, npm run e2e (which does appear to be out of date, its looking for text that I couldn't find anywhere in the app), and npm run test to work.

angular.json Outdated Show resolved Hide resolved
angular.json Outdated Show resolved Hide resolved
@frederikprijck
Copy link
Member

frederikprijck commented Sep 21, 2022

Running npm run lint throws an exception, can you look into this (should be solved if you solve the comments I made above)?

Let's also ignore e2e for now, I see what you mean. It probably wasn't intended to work.

Btw, I know this PR is abit old, and I don't expect you to keep putting effort here to get this merged. Let me know if you want me to patch it as needed for you. That said, more than happy to leave it to you, just feel free to let me know!

@Rlcolli4
Copy link
Author

Rlcolli4 commented Sep 21, 2022

@frederikprijck I'll take a look at these this afternoon. I don't mind continuing to improve this PR if it helps your team focus on getting settled with the project. I am on the west coast of the US so I apologize if these responses appear as being "late".

@Rlcolli4 Rlcolli4 requested a review from a team as a code owner September 21, 2022 18:54
@Rlcolli4
Copy link
Author

Alright fixed the linting stuff for you, it looks like I didn't run the eslint conversion correctly for some of the projects. I re-ran the conversion and stuff looks good and the npm run lint command doesn't error any more.

@frederikprijck
Copy link
Member

Thanks for the PR 🚀 . Can you resolve the conflict and then we should be good to merge.

@Rlcolli4
Copy link
Author

Merge is done

@frederikprijck
Copy link
Member

frederikprijck commented Sep 23, 2022

First of all, thanks for the PR. Really appreciate the effort here, as this has been a PR of high quality.

I had a conversation with the Angular team and concluded the following:

  • When you compile an SDK with Angular 14 and compilationMode set to partial, the SDK will only work with Angular 14+.

However, as we still need to support Angular 12 and 13, it appears to be that we have two options:

  • Create a separate version for every major version of Angular
  • Compile using the lowest supported Angular version, currently being 12.

Given we have no specific features from angular apart from a service and an interceptor, I believe we should be fine to stick with Angular 12.

However, that would mean we can not merge this PR yet, but can only do so once Angular has dropped support for Angular 12 and 13. Maintaining multiple versions comes with a cost that is not worth it for our SDK.

I feel a bit bad about that, because of the fact that this PR has been open for so long, and all I wanted was get some activity in here and progress the repository as well as get your work in. But I believe we should not merge this PR because of what is mentioned above. I hope you understand the reasoning, and my sincere apologies.

That said, we will bump everything to Angular 12, compile using partial mode and get rid of the legacy warning when compiling.

We can update this PR, drop a couple of commits and use this PR to upgrade to Angular 12, if you like. But I don't want to waste anymore of your time, if you feel like it's wasted.

@frederikprijck
Copy link
Member

Closing in favor of #735 and #734

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.

5 participants