shlomiassaf / ngrid

A angular grid for the enterprise
https://shlomiassaf.github.io/ngrid
MIT License
241 stars 40 forks source link

Upgrade to Angular V9 #79

Closed shlomiassaf closed 4 years ago

shlomiassaf commented 4 years ago

This issue will sum up the scope/work required to migrate to angular & material v9.

Upgrading the library code itself is actually the small portion of the work required.
Most of the work will be on compatibility and refactoring of code used by the build process of the library and the demo/docs application.

I will track the progress of upgrading to angular v9 (including material plugins) here.

Moved to #106

Blockers:

Non Blockers:


First working version is published: 2.0.0-rc.0, see PR, now merged to master

For anyone who wants to see the migration commits, see the v8-to-v9-migration branch


shlomiassaf commented 4 years ago

First major issue: https://github.com/angular/angular/issues/34227

Lot's of code in the library is built that way...

Another issue i'm running into while upgrading: https://github.com/angular/angular/issues/31221


As I suspected, to early to upgrade with these 2 major issues. Looks like IVY is more suitable to less complicated projects, I will keep investigating tough.

magnusbakken commented 4 years ago

I guess the Angular team didn't really expect people to do complex logic in Input property setters. Personally I always use ngOnChanges for that purpose, but I can see the appeal of doing it in the setter.

shlomiassaf commented 4 years ago

@magnusbakken When you want to react to things immediately that makes sense, especially when you want to control performance wise.

Also, ngOnChanges is angular specific and in advanced cases one might call the property directly instead of waiting for a template binding update.

In complex scenarios, I find it more difficult to reason the behaviour when working through changes.

Last point, putting all of the change handling code in ngOnChanges is sometimes to much and not clear (code wise).

Anyway, that's my take on this...

Just keep in mind, regardless of my personal approach, that the behaviour they created is not deterministic... for bound properties you get one type of behaviour and for unbound (static values) you get another... bad choice.

shlomiassaf commented 4 years ago

Just remembered in another issue I have with it...

When working with a complex UI component, such as a table, you find your self in scenarios when change detection triggers are not automatic.

For example, working with template references where the template reference was created in another change detection section.

One can always trigger the root change detector, but that's not he purpose as we want to maximize performance.

The main issue here is changing properties in code (usually @Inputs but not always) and reacting to them. This is done in code because of complex scenarios which requires creating components from code. For example a special header component within a cell header (sorting, menu buttons, etc...)

When done through code, we can raise a detectChanges() call but since it will run from the change detector of the grid it will only trigger itself it's decedents only. Any template/component it creates, which come from the outside of the grid, will not trigger a change detection cycle cause they are out of the scope of the change detector of the grid (branched out of some common parent).

This is why it's mandatory to catch it on the setter so we can trigger a change detection cycle.