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 v15 #2212

Merged
merged 24 commits into from
Dec 11, 2023
Merged

Angular v15 #2212

merged 24 commits into from
Dec 11, 2023

Conversation

JBBianchi
Copy link
Contributor

@JBBianchi JBBianchi commented Nov 14, 2023

(Reopening #2210 after messing up with git.)

Migrated Angular/Angular Material to v15 (dropped compatibility with previous versions. Could work, I didn't check.)

I didn't manage to reconfigure the test environment yet for angular-material.

Any help is more than welcome! <3

  • pnpm run build
  • pnpm run doc
  • pnpm run test (fails: trying to "rewire" tests for angular-material)
  • pnpm run test-cov (same as above)
  • pnpm run lint (will do when the rest is fixed)
  • run example
  • Tested in angular seed project v15

Next steps:

  • Remove support for deprecated @angular/flex-layout
  • Add support for Angular v16 and v17

(Dev envs: Win 10/11 - NodeJS 16.14.2 & 18.18.2)

Related to

@JonasDev17 @sdirix if you have some time to have a look, it would be great. Angular is a great framework often used in companies, it would be sad to drop support for it.

Copy link

netlify bot commented Nov 14, 2023

Deploy Preview for jsonforms-examples ready!

Name Link
🔨 Latest commit 6b87dec
🔍 Latest deploy log https://app.netlify.com/sites/jsonforms-examples/deploys/65772d79d4d21d0008543133
😎 Deploy Preview https://deploy-preview-2212--jsonforms-examples.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@JonasDev17
Copy link

JonasDev17 commented Nov 14, 2023

I've moved to using ngx-formly because I found the integration with multiple frameworks in this package challenging, especially with the rapid release of new versions and dependency conflicts (Although in my opinion, Angular is very stable, other frameworks aren't so much). I've also written about it here: #2085

In case it's helpful, here's a link to my pull request where I moved away from fxLayout to CSS, which might simplify some issues: c8027b7

I usually work with Tailwind, hence the weird css class names at times.
Maybe there is more you can take out of my PR - it is a bit of a mess though ngl.

Good luck!

@JBBianchi
Copy link
Contributor Author

I've moved to using ngx-formly because I found the integration with multiple frameworks in this package challenging, especially with the rapid release of new versions and dependency conflicts (Although in my opinion, Angular is very stable, other frameworks aren't so much). I've also written about it here: #2085

In case it's helpful, here's a link to my pull request where I moved away from fxLayout to CSS, which might simplify some issues: c8027b7

I usually work with Tailwind, hence the weird css class names at times. Maybe there is more you can take out of my PR - it is a bit of a mess though ngl.

Good luck!

I saw your PR and comments. I'll probaly go back to it when getting rid of flex-layout. I must admit I have a sweet spot for the approach of JsonForms over ngx-formly. I do understand how you feel about the monorepo, it's a bit of an obstacle. (Idk if it's me, but I'm under the impression the transition to pnpm helps over npm ... Or maybe the team already made things smoother by upgrading the other frameworks.)

Atm, still stuck at wiring the tests and I have no clue why. I wonder to which extent it wouldn't be a good idea to create an angular workspace file (angular.json) to ease up the config & upgrades by relying on angular-cli instead of trying to work around it.

@lucas-koehler
Copy link
Contributor

Hi @JBBianchi ,
thank you very much for the PR ❤️
I'll have a closer look this week.

I do understand how you feel about the monorepo, it's a bit of an obstacle. (Idk if it's me, but I'm under the impression the transition to pnpm helps over npm ... Or maybe the team already made things smoother by upgrading the other frameworks.)

We also noticed that. The switch to pnpm was one step to try to alleviate the problems. Especially Angular does not seem to play that well in a mono repo with other non-angular packages. One problem with splitting it to two repositories is again that this then increases maintenance and release efforts again taking away more time from other dev.

Atm, still stuck at wiring the tests and I have no clue why. I wonder to which extent it wouldn't be a good idea to create an angular workspace file (angular.json) to ease up the config & upgrades by relying on angular-cli instead of trying to work around it.

As part of #1546 I started to investigate how we could switch to library builds and we noticed that this does not seem to work too well in the monorepo structure. Thus, we decided it is okay if the angular packages are moved to a new folder angular at the root level of the repository. This folder can then contain its own angular workspace that draws in/links the core package from the current monorepo structure.
Unfortunately, I do not know when I'll have time to make further progress on this. But if it helps you for this PR, feel free to move the angular packages to a new root directory and configure an angular workspace.

@JonasDev17 I am sorry that you felt neglected. I appreciate that you still took the time to answer here despite having moved on from the framework. Thank you!

@JBBianchi
Copy link
Contributor Author

As part of #1546 I started to investigate how we could switch to library builds and we noticed that this does not seem to work too well in the monorepo structure. Thus, we decided it is okay if the angular packages are moved to a new folder angular at the root level of the repository. This folder can then contain its own angular workspace that draws in/links the core package from the current monorepo structure.
Unfortunately, I do not know when I'll have time to make further progress on this. But if it helps you for this PR, feel free to move the angular packages to a new root directory and configure an angular workspace.

I'll investigate during the week. On top of my mind, I see 3 possibilities to leverage angular-cli but I'm not sure of the side effects:

  • Create an angular workspace at the root of the monorepo. There might be a problem for the dependencies management because the angular workspace uses the root package.json instead of the projects' package.json. There is maybe ways to mitigate that.
  • Create one angular workspace by package. It problably implies duplicating the dependencies, but I guess pnpm have optimisation mecanisms for that.
  • Move the angular packages in a specific folder to host the workspace. I'm not sure how well it will play with the monorepo.

An other point that comes to my mind but have a wider impact on the monorepo is the use of nx. Afaik, nx have a robust toolset to manage monorepo with mixed frameworks, it might be interesting to see how much effort is required to migrate from the lerna monorepo to nx.

@lucas-koehler
Copy link
Contributor

Hi @JBBianchi ,

I'll investigate during the week. On top of my mind, I see 3 possibilities to leverage angular-cli but I'm not sure of the side effects:

  • Create an angular workspace at the root of the monorepo. There might be a problem for the dependencies management because the angular workspace uses the root package.json instead of the projects' package.json. There is maybe ways to mitigate that.

If it can be done without polluting the main package.json that could be an option. However, adding the angular config to the root package.json is not desirable in my opinion.

  • Create one angular workspace by package. It problably implies duplicating the dependencies, but I guess pnpm have optimisation mecanisms for that.
  • Move the angular packages in a specific folder to host the workspace. I'm not sure how well it will play with the monorepo.

I think this will not integrate well out of the box with the monorepo. I'd hope this could be mitigated by linking the jsonforms core into the angular workspace (e.g. as part of a pre build step in the angular workspace) to get an acceptable dev experience.

An other point that comes to my mind but have a wider impact on the monorepo is the use of nx. Afaik, nx have a robust toolset to manage monorepo with mixed frameworks, it might be interesting to see how much effort is required to migrate from the lerna monorepo to nx.

Using nx would definitely be an option for me if that alleviates the problem. Even if you don't add this in this PR, gladly report on your findings as this might also be interesting in the future :)

Also I want to give you some notes I took some time ago:

in contrast to other packages, the project in angular is not the same as the installable package.
this is only generated (with the appropriate package.json) via ng build into the dist folder.
=> it is unclear how referencing a package via pnpm would work because the package in dist should
not be part of the pnpm workspace

Maybe there is a workaround for this in pnpm or by using nx or in Angular CLI.

@JBBianchi
Copy link
Contributor Author

I managed to run the tests when creating a angular.json workspace at the root and installing @angular/cli, @angular/core and @angular/platform-browser-dynamic in the root as well.

As it was messing around with the root of the monorepo, I tried to RE the test process instead to be able to use @angular-devkit/build-angular/plugins/karma in the karma config.

So I removed that mess and created a test-runner.js that kinda mimics what angular is doing under the hood.

The good point is that the tests are running, the not so good point is that some are failing (not really a surprise), the (very) bad point is that the webpack output doesn't show. So when the build fails, it's just running 0 out of 0 test without leaving any clue about what's going on.

It's messy and not ideal but I must admit I'm already happy so see those tests running after hours spent crawling angular libs with the debugger.

@@ -156,7 +157,6 @@ export class MasterListComponent
selectedItemIdx: number;
addItem: (path: string, value: any) => () => void;
removeItems: (path: string, toDelete: number[]) => () => void;
propsPath: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When building the library, the bundler doesn't seem to complain. But when testing, an error occurs because it already exists in JsonFormsAbstractControl.

@lucas-koehler
Copy link
Contributor

Hi @JBBianchi ,
thanks for the update. That already sounds quite promising :)

As you seem to still work on that currently, could you ping @sdirix and/or me when you have concrete questions and or like a review :)? Otherwise, it's a bit hard to judge when/what to investigate because as looking at everything multiple times adds up to quite a lot of effort.

the (very) bad point is that the webpack output doesn't show.

Is that a general issue or only when the webpack build fails?

@JBBianchi
Copy link
Contributor Author

JBBianchi commented Nov 17, 2023

As you seem to still work on that currently, could you ping @sdirix and/or me when you have concrete questions and or like a review :)? Otherwise, it's a bit hard to judge when/what to investigate because as looking at everything multiple times adds up to quite a lot of effort.

Sure, I'll do.

Is that a general issue or only when the webpack build fails?

Currently, building of the library relies on Rollup (green lights) but the testing it relies on Webpack.
When running the tests, if the (webpack) build fails, the webpack output is not forwarded by karma (if it's successful, nothing is outputted anyways). Karma will then run 0 out of 0 test. It's mostly problematic as a developer because you don't know where things went south. In a CI context, it's not a big deal, by default, Karma failOnEmptyTestSuite is true, so no test == failed tests.
I'm not sure how much effort should be put in fixing this. The Angular ecosystem is shifting a bit, I'm not sure but I think jest is kind of favored now and I assume webpack will slowly get deprecated in favor of esbuild.

@JBBianchi
Copy link
Contributor Author

I'm again hitting a tooling wall with linting.

Eslint throws 'import/no-unresolved' errors for 'subpaths' (not sure of the terminology here), so basically imports from "submodules", e.g.:

error Unable to resolve path to module '@angular/core/testing' import/no-unresolved
error Unable to resolve path to module '@angular/platform-browser-dynamic/testing' import/no-unresolved
error Unable to resolve path to module '@angular/material/button' import/no-unresolved
error Unable to resolve path to module '@angular/material/icon' import/no-unresolved
...

I suspect it's related to import-js/eslint-plugin-import#1810 as in angular v12, each "submodule" had it's own package.json and now (in v15 but I don't know since which version) the "subpath" are "exported" directly from the root package.json.

@lucas-koehler
Copy link
Contributor

lucas-koehler commented Nov 20, 2023

@JBBianchi , just to confirm: The imports work but just the linter incorrectly reports errors due to the issue you linked, right?
If that's the case, you could ignore these errors for now in packages/angular-material/.eslintrc.js only for the relevant angular packages. See this comment in the issue you linked on how to do this: import-js/eslint-plugin-import#1810 (comment)

@JBBianchi
Copy link
Contributor Author

@JBBianchi , just to confirm: The imports work but just the linter incorrectly reports errors due to the issue you linked, right? If that's the case, you could ignore these errors for now in packages/angular-material/.eslintrc.js only for the relevant angular packages. See this comment in the issue you linked on how to do this: import-js/eslint-plugin-import#1810 (comment)

@lucas-koehler Yes, the imports work but the linter isn't able to understand them. I was indeed thinking about ignoring those errors.

I'm also facing issues when building the example because of the custom webpack config. I'll probably have to follow the same methodology I used for testing, aka debug how angular-cli does it and try to reproduce it outside an angular workspace.

I also tried to update to angular v16 but it's yet an other round of problems. It seems to build but tests are failing, I didn't investigate why yet. Anyhow, I assume the current version should be compatible with v16 or v17 without actually updating them, it would just have been nice if it worked without too many efforts.

@lucas-koehler
Copy link
Contributor

@JBBianchi

Yes, the imports work but the linter isn't able to understand them. I was indeed thinking about ignoring those errors.

Let's just turn it off to prevent it from blocking :)

I also tried to update to angular v16 but it's yet an other round of problems. It seems to build but tests are failing, I didn't investigate why yet. Anyhow, I assume the current version should be compatible with v16 or v17 without actually updating them, it would just have been nice if it worked without too many efforts.

I see. I suggest just ignoring the v16 update for now. "Only" the update to v15 is a huge upgrade to the current state :) I think it makes sense to focus on this getting ready rather than risking the effort blowing up too much by trying to fix it for 16 in the same PR.

@JBBianchi
Copy link
Contributor Author

JBBianchi commented Nov 21, 2023

I ignored the linting errors and fixed the example build (I used a combo of ngc, rollup and http-server instead of webpack).

(note, the CICD doesn't seem to like the example refactor ?)

One last thing that might be necessary would be to check the styling. With the MDC migration, few things have changed. I'm thinking about the background of the fields that are now a bit gray and also the icons/side nav of the master-detail renderer that seems bugged.

Edit: I guess the CICD is broken because the location of the files to deploy slightly changed. I don't think I have eyes on those "checks" and their config.

@coveralls
Copy link

coveralls commented Nov 23, 2023

Coverage Status

coverage: 83.033% (-1.4%) from 84.469%
when pulling 6b87dec on JBBianchi:angular-v15
into da3321b on eclipsesource:master.

@JBBianchi
Copy link
Contributor Author

Coverage Status

coverage: 0.0% (-84.5%) from 84.469% when pulling c1dd4b3 on JBBianchi:angular-v15 into 77624c6 on eclipsesource:master.

There seem to be an issue with the coverage report. @lucas-koehler or @sdirix could you investigate by any chance ? (same for the deploy that now fails ?)

@lucas-koehler
Copy link
Contributor

Hi @JBBianchi ,
the deploy seems to fail because the angular-material example app build ( pnpm run build:examples-app) currently fails. This is used by the deployment.
It gives an error like this:

pnpm run build:examples-app 

> @jsonforms/angular-material@3.2.0-alpha.3 build:examples-app /home/lucas/Git/jsonforms/packages/angular-material
> rollup -c rollup.example.config.js


example/dist/example/main.js → example/dist/bundle.js...
[!] (plugin rpt2) Error: Could not resolve entry module (example/dist/example/main.js).

    at error (/home/lucas/Git/jsonforms/node_modules/.pnpm/rollup@2.79.1/node_modules/rollup/dist/shared/rollup.js:198:30)
    at ModuleLoader.loadEntryModule (/home/lucas/Git/jsonforms/node_modules/.pnpm/rollup@2.79.1/node_modules/rollup/dist/shared/rollup.js:22306:20)
    at async Promise.all (index 0)

 ELIFECYCLE  Command failed with exit code 1.

Maybe you need to adjust some paths in packages/angular-material/rollup.example.config.js?

@sdirix Do you maybe have an idea what could be going on with the coverage report?

@JBBianchi
Copy link
Contributor Author

JBBianchi commented Nov 30, 2023

@lucas-koehler @sdirix I managed to fix the test coverage but at a cost. I had to use the usual 'angular' way to manage the test files which is "side-by-side" instead of storing all the tests if a separated directory.

See changes here: https://github.com/JBBianchi/jsonforms/pull/1/files

Is this acceptable ?

Edit: irrelevant, see next comment.

@JBBianchi
Copy link
Contributor Author

@lucas-koehler @sdirix I managed to fix the test coverage but at a cost. I had to use the usual 'angular' way to manage the test files which is "side-by-side" instead of storing all the tests if a separated directory.

See changes here: https://github.com/JBBianchi/jsonforms/pull/1/files

Is this acceptable ?

Nevermind, I found the solution, test coverage fixed.

@JBBianchi JBBianchi changed the title WIP: Angular v15 Angular v15 Nov 30, 2023
@JBBianchi
Copy link
Contributor Author

@lucas-koehler @sdirix Unless I missed something, this PR is ready for review.

Copy link
Contributor

@lucas-koehler lucas-koehler left a comment

Choose a reason for hiding this comment

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

Hi @JBBianchi ,
thanks for your massive effort! I had a look at the code and the changes and they look pretty good to me already. I have some comments inline, most of them pretty minor :)

Please have a look. I can test the changes probably next week :)

I could see in the build log that the coverage could in fact be calculated now but it's still reported as 0% in the (Edited) PR comment. Looking at the coveralls build https://coveralls.io/jobs/132565001 it only looks at file .prettier.js and thus reports 0% coverage. @sdirix do you have an idea what could cause this?

packages/angular-material/karma.conf.js Outdated Show resolved Hide resolved
packages/angular-material/package.json Outdated Show resolved Hide resolved
packages/angular-material/src/controls/range.renderer.ts Outdated Show resolved Hide resolved
packages/angular-material/test-config/karma-test-shim.js Outdated Show resolved Hide resolved
packages/angular-material/test-config/karma-test-shim.js Outdated Show resolved Hide resolved
packages/angular-material/test-runner.js Outdated Show resolved Hide resolved
packages/angular-material/test-runner.js Outdated Show resolved Hide resolved
packages/angular-material/package.json Show resolved Hide resolved
packages/angular-material/package.json Outdated Show resolved Hide resolved
@JBBianchi
Copy link
Contributor Author

I don't understand why the CI fails all the sudden, especially for an untouched vue thing ...

- Regenerated the lock file from scratch
- Fixed broken typings in one vue test caused by dependency updates
@lucas-koehler
Copy link
Contributor

I don't understand why the CI fails all the sudden, especially for an untouched vue thing ...

Hi @JBBianchi ,
I regenerated the pnpm-lock file again and added a fix for the vue test. I believe the one vue test broke due to some typing update in vue. I could reproduce it locally.

@lucas-koehler
Copy link
Contributor

@JBBianchi Thanks for the updates. The code changes itself look good to me now. I still want to test it though. Not sure if I can finish that today. Otherwise, my plan is to do it in the beginning of next week.

Copy link
Contributor

@lucas-koehler lucas-koehler left a comment

Choose a reason for hiding this comment

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

Hi @JBBianchi, I finally got to test the changes. I tested it in the angular seed project and a fresh project created with the CLI. It worked in both :)

Thanks again for this great contribution and all the effort you invested ❤️

I added the recent commits to fix (and improve) the coverage check by improving the coverage report merge.

@lucas-koehler lucas-koehler merged commit 18c0825 into eclipsesource:master Dec 11, 2023
8 checks passed
@JBBianchi
Copy link
Contributor Author

Great news @lucas-koehler, thanks a lot for your support !

There is one key aspect that still needs to be addressed though, the version compatibility.

With those changes, the package will only be compatible with Angular v15.

Maybe it could be a good idea to define a semver that is laxer and publish a preview version of the package ? That way, we could verify if it still work with previous versions of Angular and if it's compatible with v16 and v17.

Wdty ?

@lucas-koehler
Copy link
Contributor

Hi @JBBianchi ,
regarding Angular versions older than 15, I'd drop the support now for good: It makes it harder to maintain and Angular versions 14 and older are end of life anyway (see here).

Expanding the compatibility to 16 and 17 would be great in general. However, I'd like to test them before just publishing a new version, even if it is an alpha or beta.
Looking at the compatibility matrix, Angular 16 should at least work with the current Typescript and Node versions. For Angular 17, we need to first upgrade to Node 18 (which we want to do in #2224 ). Realistically, I won't have time to investigate the Node upgrade this year anymore :( . IIRC builds work with Node 18 already but there were some issues with test execution.

If you'd like to contribute that, you can gladly open a PR to allow Angular 16 and we'll review that :)

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.

enableIvy should not be set to false! Support for Angular 15 Angular Ivy distribution
4 participants