-
-
Notifications
You must be signed in to change notification settings - Fork 9.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
💢 Perform update to Angular 17 #2173
Comments
@bkimminich Are there specific strategies or patterns you recommend for effectively inline-replacing older modules to ensure seamless integration with Ivy? Appreciate any guidance as I'm keen to contribute/take on this issue :) |
If any additional assistance is needed, I would also be happy to help. Thanks. |
We didn't have to do this up until now, as everything was always backward compatible with NGCC. The official recommendation is to ask library maintainers to switch to Ivy, but since that doesn't help with abandoned libs, we're on our own so to speak to find a lib replacement or recreate them in our own codebase as modules/components. |
@EbonyAdder, this totally could be tackled by more than one person, so how about I invite both @AnmolSirola and you to our next developer meeting on Jan 17th and we can come up with a battle plan? I'll ping you on Slack for the invitation. |
Hey together, is this still needed? I'm quite experienced with Angular and I have upgraded already a couple of Angular app to v17 even. |
Hi! Yes, this is still an open task and we'd love to get your help with it! 👍 |
Sure :) Also since May v18 will be released does it make sense to update to v17 already as well? (of course first to 16) but then to 17 as well? |
The existing branch was more an experiment I did when I bumped into the GCC vs Ivy 🧱 ... 😅 Please feel free to ignore that one. And of course going all the way to the latest version would be ideal! 🤩 |
Sure I'll give it a try :) :) |
@bkimminich ok I was able to upgrade to v16 (after everything is fixed I'll upgrade to v17) and build it without errors.
Then another thing: Your fork: Anuglar2-qrcode also creates issues. I forked it already from you and just thinking about migrating this to Ivy as well - would that be ok with you? |
Wow, you're fast! 🤩 ... The Angular QR Code library fork is part of a hacking challenge, so if we'd inline it as an Ivy module, we will need to pick another "victim" for our typosquatting challenge. That's not a big deal, so please feel free to replace it. You can base it on the original lib, not my fork, as I only made this for the typosquatting challenge. |
@bkimminich for v16 I have created a PR: I would recommend updating angular/material in another separate branch since it affects MAAANY files :D |
Awesome, thanks! We'll check it out asap! 👍 |
An update to v17 has also landed. |
Wow! I'll look at it latest on Saturday when I'm back from a business trip. 🚀 |
@martinakraus, I'm moving any remaining issue discussions here so we don't need to always go back to the closed PR. I noticed that the minification is not happening on the https://update.angular.io/?l=3&v=15.0-17.0 doesn't give me any hint why the behavior has changed from minified-by-default to non-minified. |
The remaining issues come from the Angular configuration. In dev mode all seems to work fine, in production mode all goes up in 🔥. I tried different configuration flag permutations, but none was without errors unfortunately. The Angular migration guide didn't help me find the issue either. |
Ok I'm not quite sure. |
Just |
Ah I have actually a guess. Additionally I read on the docs
|
Yep - it is totally related to my moving dependency things. Btw npm does print a warning: so maybe we could switch that in CI/CD as well:
|
I think we tried that once, but that isn't recognized by Angular iirc, so let's first try to get back to "works as before" and then we can still consider optimizing. |
So with this package,json: The npm install --production (sorry for over-optimizing 😅) So the npm install --production went through on the docker smoke test. |
How do you run the production build? |
I just run
and on
which is a non-prod build. If you try |
I see - there seems to be something a bit wrong with the configuration. Usually ng build is already prod build, which has |
Indeed
|
Hi @martinakraus, I think I've pulled all the command line arguments into the |
I'm so sorry😢 sometimes life happens and work keeps crushing me. As mentioned moving to standalone components seems to resolve the issue on aot compilation. It's not that big of a deal. It just needs to be done carefully with some time. |
No worries, I just removed that label because I thought that the current state of Angular-affairs is not really something for a beginner to pick up... 😁 I was coincidentally just playing around with standalone components:
I was down to one remaining compiler error for that component at least... But I'm not entirely sure this is "right" and would result in a running application or if it would just compile and then the disaster happens at runtime. |
⏰ Getting this issue resolved with community support would be amazing! I currently consider this the sole blocker for releasing |
I upgraded an old Angular application by like four versions before, and this was pretty recent. I think I can help. I have never contributed to open source before, so I am reading the contribution guidelines for the project now. |
I am caught up on reading and have forked the repository.
Have these changes been committed somewhere, or should I start working from 'develop'? |
The working branch is |
I will do my best. |
Just taking a look with a fresh perspective is already worth a lot... 🙏 As the CI/CD is currently in shambles, I ran the frontend unit tests locally, and there are some issues with several components apart from the Customer Feedback screen. Also problems with the About Us page, the code snippet dialog and some others. All in all my local result of
|
I'm seeing 54 failed on my end; much of what you have described though. I am investigating; I think I had a similar issue with npm packages when I did the migration at my last job. |
When running npm test I received many "Error: NG01203: No value accessor for form control unspecified name attribute." It appears from my limited testing that specifying ngDefaultControl for UntypedFormControls may resolve this issue. Of course, I will need to test this again running the application and with e2e tests. Mentioning here because you may already know whether this is an acceptable solution or if a more tailored approach will be needed for each UntypedFormControl, but I will continue investigating. I received another error suggesting that While investigating I am down to 46 failures, but I see many instances of those same errors described above. |
If I apologize if this is too verbose for this thread, this is just my first time contributing to open source as I mentioned before, and I want to make sure that these findings are documented in a timely manner so that we are not needlessly duplicating effort. I will continue applying my findings to other components and working to resolve these errors. |
This is by no means too verbose, it's super helpful! 🙏 I wasn't even considering the possibility that the script might have overlooked some existing Material component... 😅 |
I went down a rabbit hole with npm packages and then got a little distracted. There will have to be some legacy-peer-dependencies, I'm sure - as there already are - so I'm doing a bit of a technical triage of the situation and deferring as much of that as I can justify while I get the UI cleaned up and tests running. With the last migration I did, I had to do a small patch on a dependency with the help of https://www.npmjs.com/package/patch-package, and that might be acceptable for this use-case if it comes down to it; but hopefully it won't - I'm just mentioning it here as a last-resort option for anything we cannot otherwise resolve in a timely manner. Even The routing should probably be updated at some point - in the tests as well - so I'm looking into that a little bit. I will continue on it tomorrow; it is currently 1 AM in my timezone, so I should get to sleep. |
I still need to finish fixing the UI, but I have been trying to get these unit tests running. I made a couple of commits to my fork here, for your review: https://github.com/thomasbreland/juice-shop/tree/2173_breland The CodeSnippetComponent and ScoreBoardPreviewComponent tests seem to be the only ones having issues; 25 total test failures, but really just the same two recurring issues. I am of course very new to this repo, so I want to make sure I that really understand what is going on before I start editing tests/mocks/spies - lest the tests only confirm existing behavior. In score-board.component.spec.ts
This seems to be defined with a spy of ConfigurationService elsewhere when tested, e.g., challenge-solved-notification.component.spec.ts:
In code-snippet.component.spec.ts
The line referenced occurs within the ngOnInit function: The variable is declared in code-area.component.ts:
|
After being stuck on the tests or a while, I decided to focus on the UI for a bit. Running the
I also noticed that it missed renaming one css element, but I fixed that and committed those changes to my fork. Now I have to figure out which internal classes no longer exist, and I'm also trying to get the search bar to better match the old A number of the modules have also been renamed to xModule, where x includes - but is not necessarily limited to - the following:
I have made some progress with the cards locally, but there is still work to be done. |
Wow, I really wasn't expecting this to become that big of an endeavor... 😨 If you'd like to see the CI/CD results for your branch, feel free to just open a PR (in draft mode for example), and then most of the tests will trigger whenever you push something new into the PR's underlying branch. |
I did a bit more of the MDC migration, took a break, and came back to it. I want to make sure that everything is consistent and that I'm really understanding the differences between the old and new components before I start committing more changes that I might soon revert. Still, here are some things that might be interesting.
Sometimes there just isn't a way around it, but I'm trying to minimize unnecessary changes.
The impact of this was immediately obvious. I think the easiest refactor would be to just place the card contents in a div with set padding, which should hopefully avoid breaking changes in the future, but I'm trying a few different ways and I'm open to suggestions. No way around it, we will need a serious code review when the time comes, but hopefully this is enough information to help if anyone else has strong opinions about the style code and would like to give it a shot. Still, I will keep working on it and reporting my findings. Other observations:
The last commit I pushed to my fork was just changes to the scss material theme. A side effect of typography not being defined in the styles was that certain custom properties expected by Material were not defined, like "--mdc-filled-text-field-container-color" and "--mdc-filled-text-field-disabled-container-color" which determine the background color of e.g., the search bar. |
I have opened my first ever draft PR. The test failures mentioned in #2173 (comment) still remain, unfortunately. I finished migrating the cards and tables, and I've included a checklist for the remaining issues I'm working on.
There also seems to be a bit of styling that is no longer in use, but that might best be handled as a separate task. Most of these things should be pretty simple, it's just a matter of verifying that there are no side-effects and keeping the SCSS succinct. |
I am unable to recreate the errors occurring in the installation step of the pipeline locally. Looking at the pipeline, I notice the following...
...even while the pipeline step indicates Node.js 18; this might be worth looking into. It is possible that we need to update the pipeline actions themselves. It might also help to bring back the package-lock.json and use 'npm ci' instead of 'npm i' to ensure that the pipeline is using the exact same package versions. (I'm guessing that clearing the cache would not help - considering that we never call https://github.com/actions/cache - but it is another troubleshooting suggestion that I have run across.) |
In order to stay up to date, Juice Shop needs to get updated to Angular 16. There is an initial attempt available on the
angular16
branch, but it fails from some used modules not being compatible with the Ivy compiler. Angular removed the backward compatible compiler which worked with these older modules, and now Ivy is the only compiler available from Angular 16 forward.We probably need to decide for some modules if we want to inline them or replace them with more up-to-date alternatives. At least for the
ng-simple-slideshow
we need to make sure that replacing them does not accidentally break XSS challenges.Recommended Links
The text was updated successfully, but these errors were encountered: