jupyter-widgets / ipydatagrid

Fast Datagrid widget for the Jupyter Notebook and JupyterLab
BSD 3-Clause "New" or "Revised" License
579 stars 51 forks source link

Update to ipywidgets 8 #282

Closed ibdafna closed 2 years ago

ibdafna commented 2 years ago

Signed-off-by: Itay Dafna i.b.dafna@gmail.com

ibdafna commented 2 years ago

@martinRenou I seem to be getting an type issue in the ipywidgets library when compiling ipydatagrid, really odd:

/node_modules/@jupyter-widgets/base/lib/widget".WidgetView' has no exported member named 'InitializeParameters'. Did you mean 'IInitializeParameters'?

Wondering what's going on. Can you see anything off with my updates?

martinRenou commented 2 years ago

@ibdafna You should probably rename it to IInitializeParameters for it to compile. This structure has been renamed in https://github.com/jupyter-widgets/ipywidgets/commit/5221e4dd3a831739a7408ed1afd10311650412ad#diff-efb19099381ae8911dd7f69b015a0138d08da7164512c1ee112aa75100bc9be2R694

ibdafna commented 2 years ago

@ibdafna You should probably rename it to IInitializeParameters for it to compile. This structure has been renamed in jupyter-widgets/ipywidgets@5221e4d#diff-efb19099381ae8911dd7f69b015a0138d08da7164512c1ee112aa75100bc9be2R694

Yeah, I'm not sure why this is happening because it's coming from @jupyter-widgets/base package on the latest beta version. Perhaps the rename is not reflected in the release?

EDIT: quite silly of me - I didn't rename this in the extension file.

ibdafna commented 2 years ago

@martinRenou I took another stab at fixing the jest spyOn() issue. I get a more meaningful error message now and it seems to be related to our mock comm of the mock widget manager.


 FAIL  tests/js/datagrid.test.ts
  ● Test suite failed to run

    tests/js/datagrid.test.ts:37:29 - error TS2769: No overload matches this call.
      Overload 1 of 4, '(object: {}, method: never): SpyInstance<never, never>', gave the following error.
        Argument of type 'IClassicComm | undefined' is not assignable to parameter of type '{}'.
          Type 'undefined' is not assignable to type '{}'.
      Overload 2 of 4, '(object: {}, method: never): SpyInstance<never, never>', gave the following error.
        Argument of type 'IClassicComm | undefined' is not assignable to parameter of type '{}'.
          Type 'undefined' is not assignable to type '{}'.

    37     const mock = jest.spyOn(grid.model.comm, 'send');
                                   ~~~~~~~~~~~~~~~

Test Suites: 1 failed, 1 total
Tests:       0 total
Snapshots:   0 total
Time:        3.469 s```

https://github.com/bloomberg/ipydatagrid/blob/2540fcf8edc946c00c218537114830dc0ad16bf4/tests/js/testUtils.ts#L155-L218
ibdafna commented 2 years ago

galata update references

ibdafna commented 2 years ago

update galata references

ibdafna commented 2 years ago

update galata references

ibdafna commented 2 years ago

@martinRenou It seems like the Galata bot has updated the reference images with a images from a failed render attempt - they all show "error displaying widget model". The reference images are captures using ipywidgets 7, and testing locally with ipywidgetx 7.x works fine. Another thing to think about is that there we only capture reference images from one source - either the 7.x or 8.x environment. Should we think about extending the logic so we have separate images for both versions? Is it even reasonable to expect images to render (however slightly) differently between the two major versions?

ibdafna commented 2 years ago

update galata references

martinRenou commented 2 years ago

they all show "error displaying widget model". The reference images are captures using ipywidgets 7

Are you able to capture the JS outputs for those tests? Jason shared some nice debugging tricks with Galata here https://github.com/jupyter-widgets/ipyleaflet/pull/968#issuecomment-1219673451

Is it even reasonable to expect images to render (however slightly) differently between the two major versions?

I guess it's reasonable. I've seen a slight shift to the right for some outputs in ipympl and bqplot, but I bet it's due to some change in Galata or JupyterLab, not ipywidgets.

ibdafna commented 2 years ago

@martinRenou the visual regression tests are genuinely failing on ipywidgets 7.x. We didn't think about the JupyterLuminoPanelWidget imports in ipywidgets 7.x. I added a function with some ts-ignores, but it's still failing. Need to investigate further

ibdafna commented 2 years ago

update galata references

ibdafna commented 2 years ago

update galata references

ibdafna commented 2 years ago

update galata references

ibdafna commented 2 years ago

update galata references

ibdafna commented 2 years ago

update galata references

martinRenou commented 2 years ago

update galata references https://github.com/bloomberg/ipydatagrid/actions/runs/3153162984

martinRenou commented 2 years ago

Now that we have two bots for 7 and 8, I'm a bit worried that the bot that will take the longest time to run will fail to push the commit, it will probably need to pull first.

ibdafna commented 2 years ago

Even if we have two separate directories?

ibdafna commented 2 years ago

If it ends up being too difficult to set up two bots, I may just give up and revert to a single bot with tests on either ipywidgets 7 or 8. It's nice to be covered on both ends, but maybe it's not the end of the world if we only do visual regression testing with one version instead of two.

ibdafna commented 2 years ago

🎉🎊🥳