shlomiassaf / ngrid

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

In version 4.0 createRowEvent function sometimes crashes due to event.context == null #250

Closed t00 closed 1 year ago

t00 commented 1 year ago

I could not find any workaround to this problem, it happens when hovering over grid rows after selecting an active row (implemented using getRowClass) a couple of times, then hovering at some point breaks the grid in the same code location:

  const matrixPoint = matrixRowFromRow(rowTarget, cdkTable._rowOutlet.viewContainer);
  if (matrixPoint) {
      const event = Object.assign(Object.assign({}, matrixPoint), { source, rowTarget });
      if (matrixPoint.type === 'data') {
          event.context = this.pluginCtrl.extApi.contextApi.getRow(matrixPoint.rowIndex);
          event.row = event.context.$implicit;
      }

Above, event.context is null thus getRow function just returned null. Going further, inpebula-ngrid.js this.viewCache is empty when trying to use it:

rowContext(renderRowIndex) {
      return this.viewCache.get(renderRowIndex);
  }

It might be hopefully safer not to assume row is found (event.context can be null).

I tried to narrow down the problem but for some reason it happens randomly. In my typical test: I have 1st row active on view init (using getRowClass function to determine a class of row being active). I then select 3rd row and hover back to 2nd - crash happens every time. We have [vScrollFixed]="30" - bug can be triggered by non-standard row heights.

Hope some kind of fix can be prepared without a reproducible demo - the code suggests that viewCache is not guaranteed to be present hence event.context can be undefined.

Is there a way to hook into contextApi.getRow or even better fromEvent(cdkTableElement, 'mousemove') or createRowEvent so that bugs like this one can be fixed without a release cycle?

By the way, ngrid repository has been quiet for a few months, is everything OK or just stable enough not to change it?

t00 commented 1 year ago

I tried to reproduce the issue by copying relevant code, exact angular/ngrid versions etc but it just won't break! You can find the code here: https://stackblitz.com/edit/pebula-ngrid-starter-zaj1u2

t00 commented 1 year ago

Update: in the stackblitz above, clicking on one or more rows should reproduce the issue. It must be related to either dynamic column updates or syncRows during hovering.

t00 commented 1 year ago

Any updates on the problem? event.context is not defined error is shown after clicking, when hovering over rows - Chrome, Firefox are definitely affected.

t00 commented 1 year ago

@shlomiassaf hope you did not abandon ngrid?

shlomiassaf commented 1 year ago

Hi

Sorry for being MIA.
Since nGrid was pretty much working I did not pay the attention I should have to the project.

My current position brings some challenges here, in terms of time management and resources.

This is why we're currently training internal employees in our company to help maintain and progress nGrid.
We'll try to push the quick wins and create a road map moving forward, and I'll hope i'll be able to share it for transparency

nGrid is a production component in our stack so we have much interest fixing and progressing it.

Thanks for understanding, i'll keep you guts posted and we'll look at this in the upcoming week or two (it's holiday season here).

Thanks!

t00 commented 1 year ago

Hiya, many thanks for good news. Indeed nGrid is working great but typically Angular moves forward and unfortunately bugs are there (for this one I added PR but it fails to compile due to branch name for some reason).

The fact you are using nGrid in production is reassuring and as from a start it's quality and cdk integration is amazing.

Have a great time off, thanks.

shlomiassaf commented 1 year ago

@t00 Is this one solved by #252 ?

I know you wrote it's a temp fix, but it seems to be the fix since it happens when there is no row.

nGrid will try to find the row but sometimes the hover happens where there is no physical row, the fix should be ok.

Please close if this is the case.

t00 commented 1 year ago

Yes the PR is for this bug, I added "temp" only because I don't know code well enough to determine that there should be an event with mouse present over a grid, which ends up in the handler not picking up a row. It makes sense if internal cache of a grid is re-generated during mouse move - events can be just dropped for the time being. Since you just merged the PR, I am closing it. Thanks for handling the broken merge - not sure why it failed to compile, likely I needed merge my fork's master branch.

shlomiassaf commented 1 year ago

The merge failed due to a bug in the actions checkout process. Basically, back in the day when NX was new it was an issue since we run a script to get the "affected" code and run only tests for it.

They've fixed it since, with a new NX specific action, so all good