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

Upgrade to Angular V9 #79

Closed
16 of 19 tasks
shlomiassaf opened this issue Feb 18, 2020 · 4 comments · Fixed by #80
Closed
16 of 19 tasks

Upgrade to Angular V9 #79

shlomiassaf opened this issue Feb 18, 2020 · 4 comments · Fixed by #80
Assignees
Labels
comp: build & ci build and CI infrastructure comp: core @pebula/ngrid (core package only, excluding secondary packages) comp: core-plugins @pebula/ngrid/* (All things related to core plugins the come as secondary package in @pebula/ngrid) comp: docs All things related to documentation content comp: docs-infra All things related to the documentation app incl. it's build process but excluding doc content comp: material @pebula/ngrid-material type: refactor
Milestone

Comments

@shlomiassaf
Copy link
Owner

shlomiassaf commented Feb 18, 2020

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.

  • Update dependencies (angular v9, material v9, CLI, ng-packger, TS, webpack, etc...)

  • Make it run in dev mode, explore issues and quirks.

  • Turn IVY flag off (Use ViewEngine) and verify that:

    • Demo project in Dev mode build works
    • Demo project in Prod mode build works (GH-PAGES, verify SSR still works)
    • Library build works
    • Update the Starter Project to use angular 9 with the built library and verify it works. (OPTIONAL, for safety)
  • Turn IVY Flag on and compile while fixing issues.
    Pay attention to:

    • Webpack plugins used to build the DEMO / DOCS application
    • ngx-build-plus plugins used in the app build process
    • ng-packger compatibility with the task extensions / transformers (ng-cli-packagr-tasks). Especially verify the SCSS build tasks and try to deprecate the copy assets tasks in favour of the built in copy feature of ng-packger
  • Go over all TODO references in the code and see if the "TODO" can be applied. This usually refer to:

    • Material plugins (mostly Drag & Drop), with legacy support for v7 and v8. Remove that support.
    • Material plugins inheritance, wasn't doable before should be doable now.
    • TypeScript language features that weren't possible before, but now with 3.7 are valid.

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 shlomiassaf self-assigned this Feb 18, 2020
@shlomiassaf shlomiassaf added comp: build & ci build and CI infrastructure comp: core @pebula/ngrid (core package only, excluding secondary packages) comp: core-plugins @pebula/ngrid/* (All things related to core plugins the come as secondary package in @pebula/ngrid) comp: docs All things related to documentation content comp: docs-infra All things related to the documentation app incl. it's build process but excluding doc content comp: material @pebula/ngrid-material type: refactor labels Feb 18, 2020
@shlomiassaf shlomiassaf added this to the 1.0.0 milestone Feb 18, 2020
@shlomiassaf
Copy link
Owner Author

shlomiassaf commented Feb 19, 2020

First major issue: angular/angular#34227

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

Another issue i'm running into while upgrading: angular/angular#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
Copy link

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
Copy link
Owner Author

@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
Copy link
Owner Author

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.

@shlomiassaf shlomiassaf modified the milestones: 1.0.0, 2.0.0 Feb 26, 2020
shlomiassaf added a commit that referenced this issue Mar 13, 2020
* remove tsickle

* upgrade to latest 8 version

* upgrade angular to v9

* upgrade ngx-buid-plus

* upgrade material & layout

* upgrade nguniversal

* upgrade nrwl nx

* fix type issues

* workaround unsupported ngx-cache lib

* align with v9 changes

* sync with v9

* fix gh-pages DOC site build + SS PreRender

* disable transpose matchTemplate because logic doesnt work with v9

* make lib build work and publish version

* remove cdk-drag v7 compatibility code

* fix(ngrid/drag): sneaky issue with PblDragDrop and AOT

* update readme

* refactor(ngrid): upgrade to angular 9

Partial work of #79
@shlomiassaf shlomiassaf reopened this Mar 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp: build & ci build and CI infrastructure comp: core @pebula/ngrid (core package only, excluding secondary packages) comp: core-plugins @pebula/ngrid/* (All things related to core plugins the come as secondary package in @pebula/ngrid) comp: docs All things related to documentation content comp: docs-infra All things related to the documentation app incl. it's build process but excluding doc content comp: material @pebula/ngrid-material type: refactor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants