Example 22 keeps formatter components alive outside of grid

[zewa666]

Describe the bug

It seems that custom formatters with angular, so asyncPostRenders, are polluting the DOM.

Reproduction

• Navigate to https://ghiscoding.github.io/Angular-Slickgrid/#/angular-components
• Look below the grid JohnPaulPaulPierrePaulPaulJohnPaulPaulPierrePaul
• Scroll the grid a bit down
• You'll notice the rendering below to increase for every scrolled in, thus newly created formatter

Expectation

Should not be kept as a clone

Environment Info

angular-slickgrid@6.2.1

Validations

• Follow our Code of Conduct
• Read the HOWTO - Step by Step.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion.
• The provided reproduction is a minimal reproducible example of the bug.

[zewa666]

So as far as I understand the approach, you're rendering an angular component to DOM and afterwards simply duplicating its innerhtml to the cells innerhtml.

That means 2 things:

• the original rendered component needs to be placed in a hidden container or hidden by the hide callback, thus enforcing the user to implement it.
• after copy the original ideally the component would get destroyed, which is indicated by the destroy hook of the wrapper but actually not happening.

  Angular-Slickgrid/src/app/examples/custom-angularComponentEditor.ts

  Line 133 in f5ab23a

  if (this.componentRef?.destroy) {

Its late over here, but if you dont have any concrete idea where the issue is located I can try to dig in tomorrow in the evening

[ghiscoding]

hmm I wasn't aware of that, I got so many ports it's hard to test every little detail but the creation of dynamic Angular component is all in angularUtil.service.ts which is also used by the Row Detail feature. There were some changes I've done recently to support Angular 16 in this commit since the original code got deprecated and is no longer supported, not sure if that has any effect, to tell you the truth I never used this feature neither the Row Detail because I always stick with JS custom formatters since that is synchronous and the ideal solution

[zewa666]

yeah I got the idea why classic formatters are preferrable. trouble is though if based on the field and value an async value must be looked up.

worst case one could create a mapped field where the intended value is already available through joins/maps and reuse that.

but back to the issue itself. so the underlying bug is that the element is actually supposed to be deleted but that doesnt work for whatever reason right? this way I have at least a bit of guidance of what to aim for while looking for a fix

EDIT: guess we need to look into removing it via ViewContainerRef.remove instead CompinentRef.destroy which might keep it around if there is still a ref around

[ghiscoding]

hmm there might be the enableAsyncPostRenderCleanup grid option that can be used for cleaning/destroying, it's demoed in this SlickGrid Example 10 - async-post-render-cleanup, but since I never use the post renderer, I never really investigated what this option really bring to the table but it might help to cleanup the rows that are no longer displayed (for example when scrolling since SlickGrid only caches the displayed rows)

[ghiscoding]

@zewa666 I got a question to some code change you've done in the past, so I recently migrated the original SlickGrid to TypeScript and one person is complaining that the field is now over-constrained because of the code you've introduced some time ago, it doesn't support unknown field at build time (only available at run time) as per this SlickGrid issue, would there be a way to make it work with dynamic string?

The code you introduced back then was field: Join<PathsToStringProps<TData>, '.'>;

I told him that if he wants to use interfaces, then he should extend Column and customize it to his need, but it would be nice if there's a way to make it work with dynamic string as well?

The code

type PathsToStringProps<T> = T extends string | number | boolean | Date ? [] : {
  [K in Extract<keyof T, string>]: [K, ...PathsToStringProps<T[K]>]
}[Extract<keyof T, string>];

type Join<T extends any[], D extends string> =
  T extends [] ? never :
  T extends [infer F] ? F :
  T extends [infer F, ...infer R] ?
  F extends string ? string extends F ? string : `${F}${D}${Join<R, D>}` : never : string;

interface Column {
  field: Join<PathsToStringProps<T>, '.'>;
}

I guess he could do something like this to bypass the problem

columnDefinitions: Column<MyItem & { [field: string]: string; }>[];

[zewa666]

I'm currently re-evaluating your slickgrid wrappers for a larger rewrite of an angular app in my company. Part of that might include something similar due to the mostly generic nature. So let me read up on that and I'll reply in the other issue with either a solution or more questions.

[zewa666]

@ghiscoding hey quick question, how are you building and getting angular-slickgrid ready to work with? I've tried installing the master branch via pnpm but end up with various issues once I run npm start afterwards. Perhaps you could add a super quick & dirty instruction to your readme on that?

[ghiscoding]

I'm not exactly sure what you mean, on the project itself I use Yarn classic and for the plugin build I use packagr which is the most known lib for packaging plugins. Do you mean some kind of contribution guide? If so, yeah it looks like I missed adding one

[zewa666]

well, essentially I'd like to install deps and run the start script, so that once I change things around I can live-view the examples to see the effects.

Currently I do all of these errors

X [ERROR] File 'src\main.ts' is missing from the TypeScript compilation. [plugin angular-compiler]

  Ensure the file is part of the TypeScript program via the 'files' or 'include' property.


X [ERROR] File 'src\polyfills.ts' is missing from the TypeScript compilation. [plugin angular-compiler]

    angular:polyfills:angular:polyfills:1:7:
      1 │ import 'src/polyfills.ts';
        ╵        ~~~~~~~~~~~~~~~~~~

  Ensure the file is part of the TypeScript program via the 'files' or 'include' property.


X [ERROR] TS2307: Cannot find module 'multiple-select-vanilla' or its corresponding type declarations. [plugin angular-compiler]

    src/app/examples/grid-clientside.component.ts:5:37:
      5 │ import { MultipleSelectOption } from 'multiple-select-vanilla';
        ╵                                      ~~~~~~~~~~~~~~~~~~~~~~~~~


X [ERROR] TS2307: Cannot find module 'flatpickr/dist/types/instance' or its corresponding type declarations. [plugin angular-compiler]

    src/app/examples/grid-editor.component.ts:4:46:
      4 │ ...stance as FlatpickrInstance } from 'flatpickr/dist/types/instance';
        ╵                                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


X [ERROR] TS2307: Cannot find module 'multiple-select-vanilla' or its corresponding type declarations. [plugin angular-compiler]

    src/app/examples/grid-state.component.ts:3:37:
      3 │ import { MultipleSelectOption } from 'multiple-select-vanilla';
        ╵                                      ~~~~~~~~~~~~~~~~~~~~~~~~~


X [ERROR] Could not resolve "node_modules/flatpickr/dist/flatpickr.min.css"

    angular:styles/global:styles:2:8:
      2 │ @import 'node_modules/flatpickr/dist/flatpickr.min.css';
        ╵         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  You can mark the path "node_modules/flatpickr/dist/flatpickr.min.css" as external to exclude it from the bundle, which will remove this error.

so install + start script seems not to be enough, there are some steps missing

[ghiscoding]

I added the Contributions steps, I added Tasks in VSCode (via tasks.json) and that is what I typically use most of the time, they are just short keys to the actual scripts. I don't have these problems you mention when using Yarn classic. I might switch to pnpm in the future, it's better at showing these kind of errors

[ghiscoding]

X [ERROR] TS2307: Cannot find module 'multiple-select-vanilla' or its corresponding type declarations. [plugin angular-compiler]

src/app/examples/grid-clientside.component.ts:5:37:
  5 │ import { MultipleSelectOption } from 'multiple-select-vanilla';

That one is my lib, I rewrote multiple-select jQuery 3rd part lib into a native vanilla lib, and I thought I had fixed all TS Types issues on that lib but it's kinda hard to play well all the time with TypeScript and ESM/CJS, here is the link for the package.json of that lib: https://github.com/ghiscoding/multiple-select-vanilla/blob/db1e85176237d84c5df29fc588e939d4907a5431/lib/package.json#L4-L22

if you think my exports are wrong, please let me know but apart from that I don't understand why you have all these errors, I don't see any of them when using Yarn classic on multiple computers (4x different computers in my case)

[zewa666]

I dunno, perhaps it just doesnt like windows :( but may I ask you to do a fresh clone of the repository to a different folder and see whether install + start work there? I dunno I really can't believe I'm seeing all these crazy issues.

[ghiscoding]

I'm exclusively on Windows but yes I have the project on my computers for a while, not fresh copies. I can try that.

Have you tried with Yarn classic at all?

[zewa666]

yep. everything with pnpm and yarn classic, always with fresh checkouts.
At least I found one of those bugs. It's no longer necessary to reference the polyfills.ts file so instead the angular.json section should read:

"polyfills": [
   "zone.js"
],

[zewa666]

ok got it running now. The main issue was

"architect"."build"."builder": "@angular-devkit/build-angular:browser-esbuild",

switching back to "builder": "@angular-devkit/build-angular:browser", fixed it for me. So I really wonder how on earth this worked at all for you 🤣

[ghiscoding]

well I don't know but I still have no error after a fresh copy and these kind of errors would have shown up in the CI since I do a website prod build then I use a simple http server to run the Cypress E2E tests in CI

yarn build (no error)

yarn start (also no error)

lol I don't know why we get different results 😆

I wouldn't mind receiving a PR for the polyfill change though

[zewa666]

yep, I found the issue for the attached components and just finalizing the Cypress tests. After that you'll get a PR including the polyfills fix

[ghiscoding]

you can also remove the esbuild if it's causing issue, especially for external contributors like you, better play safe and keep these experimental stuff for the future 😆

[ghiscoding]

@zewa666
There's another problem in the Example 22 and I'm not sure if you've noticed it or not, I'm not sure if it's a big deal or if it's because of the 3rd party lib I chose to test it with (ng-select in that case) but for a very short period of time it shows the ng-select ng-template for every cell being interpreted and before your fix I think it was showing like 10 of them but now it's only 1 since you destroy the previous ones, at least it's better than before. I did see that a while ago but never spent much time investigating it but it looks like the ng-template shows up in the DOM before being transitioned into the grid cell, I'm not sure if we need some hidden CSS somewhere to avoid seeing these pre-render in the DOM, it still shows in the online demo, perhaps it's because I think we have setTimeout to wait for a lifecycle, that might be the cause of why it happens quickly (transitioned on next cycle into the grid cell, it might be it)

I tried with ChangeDetectorRef and this.cd.detectChanges(); (or markForCheck()) but that doesn't seem to be enough for it to detect the change in the DOM

Maybe something like this SO Prevent ng-template from being rendered in transclusion

it happens very fast, I only managed to see it after adding a breakpoint for a tree change, these temp ng-template are what will show in the 3rd column "Assignee with Angular Component". I don't I had that problem originally and I think it's recent

[ghiscoding]

@zewa666
I found how to deal with it, so I created a new issue #1273 and a PR #1273 but it will require a new release. However, it not only fixes the UI flickering shown above, it improves the perf by 2x for every single cell which is huge because I dropped the need for setTimeout 🚀

It would be really nice if you could do a review of the PR, I still need to add unit tests to cover the new change but I'll do that tomorrow