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

Feature/angular 19 #91

Merged
merged 5 commits into from
Jan 15, 2025
Merged

Feature/angular 19 #91

merged 5 commits into from
Jan 15, 2025

Conversation

Tugark
Copy link
Contributor

@Tugark Tugark commented Jan 7, 2025

This PR holds the Angular 19 update alongside other dependencies (i.e. #62, #45, #70), because I did not want to pollute the renovate PR (and they all need to be within one and the same PR :)). It basically updates typescript, zone.js, Angular + all its libraries as well as ngrx.

Major changes:

I did some tests and it seems to work the same and look the same - but as always: Test it rigorously, since Material updates tend to break stuff as well, and I'm not sure I tested all edge cases with the new CSS fixes :)

Open issues

  • Even after clearing all caches, I still get Angular 18 inspections - this is most likely a configuration issue on my part, but be aware of it:
    image

  • We cannot update @ngrx/eslint-plugin because it conflicts with a peer dependency of eslint-plugin-rxjs-angular. This dependency is also what restricts us from updating to eslint v9; so we essentially are forced to handle this now. Our options are:

    • Override peer dependencies - which I would not, since it requires more steps and it's not really a clean fix
    • Remove the abandoned plugin
    • Use the fork (and by doing that, we also need to update to v9, which is yet another large task)

In my opinion, I think we should remove the plugin for now, merge this PR (with updated ngrx plugin) and then move to eslint v9 in a separate branch with one of the forks.

@Tugark
Copy link
Contributor Author

Tugark commented Jan 7, 2025

@wendlans @TIL-EBP Ah, pipeline tests seem to fail as well :)

Copy link
Contributor

@wendlans wendlans 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 very much for the extensive PR description - that really helped to understand the changes (apart from the the update itself).
As for this review: I can approve this PR as it is with the following suggestions:

  1. I'd say we remove the package eslint-plugin-rxjs-angular for now as you already suggested yourself. And re-add it in a separate branch which should update Eslint to 9. It would be wise to create this Eslint-Update branch ASAP - otherwise we'll forget about those rules.
  2. Any reason that e.g. @angular-eslint/schematics was not updated to 19? Similar question with @angular-eslint/template-parser, angular-oauth2-oidc
  3. Shouldn't typescript have been updated as well?
  4. Fix the pipeline 😉
  5. Remove the following lines from the .eslintrc.json file. Otherwise Eslint isn't working anymore. At least on my machine.

{
"files": ["*.ts"],
"extends": ["plugin:@ngrx/recommended-requiring-type-checking"]
}


About your Angular 18 inspection comment: I couldn't reproduce this on my machine. My IDE already detects Angular 19 and does not show false positiv errors regarding e.g. wrong inputs on standalone components.

@Tugark Tugark force-pushed the feature/angular-19 branch from db0c51a to 7c723df Compare January 14, 2025 07:30
@Tugark
Copy link
Contributor Author

Tugark commented Jan 14, 2025

@wendlans Thanks.

I adjusted your feedback - I forgot about the @angular dependencies. angular-auth-oidc was not updated in the last Angular update already, it should not have worked :D I updated it now, but can you have a look at our renovate config as to why there never were any updates regarding this plugin?

Typescript cannot be updated because @arcgis breaks due to their way of exporting stuff.

As for the pipelines - they cannot be fixed right now, there is another PR open which I will tackle in the next sprint, so this PR cannot be merged until then - or we just merge it, since the tests run locally ;)

@Tugark Tugark force-pushed the feature/angular-19 branch from 7458c74 to a6b3336 Compare January 14, 2025 14:34
@Tugark Tugark requested a review from wendlans January 14, 2025 14:39
Copy link
Contributor

@wendlans wendlans left a comment

Choose a reason for hiding this comment

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

Looks all good now, thanks for the adjustments 👌

@Tugark Tugark merged commit 80c4ad6 into develop Jan 15, 2025
8 checks passed
@Tugark Tugark deleted the feature/angular-19 branch January 15, 2025 08:10
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.

2 participants