-
Notifications
You must be signed in to change notification settings - Fork 60
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 ag-grid, perform yarn upgrade #1094
Conversation
00d423c
to
dcf6313
Compare
dcf6313
to
f7cb1e9
Compare
f7cb1e9
to
c0de37b
Compare
EF IP Tickets opened for the dependencies failing the license check:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good to me. I tested with Events Table and Latency Table. I have comment though which I'd like you to fix.
Please open a tracker for the disabled unit tests to not forget.
Thanks
|
c0de37b
to
75a1898
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested the new version and it looks good. Could you please re-arrange the commits to logical increments that work independently? Then I'll approve it. Thanks
c946c61
to
d645971
Compare
done - down to 3 commits that I think are logical increments |
Bumps [ag-grid-community](https://github.com/ag-grid/ag-grid) from 28.2.1 to 32.0.1. - [Release notes](https://github.com/ag-grid/ag-grid/releases) - [Commits](ag-grid/ag-grid@v28.2.1...v32.0.1) --- updated-dependencies: - dependency-name: ag-grid-community dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> Adapt to later ag-grid version Dependabot opened a PR to suggest we update our "ag-grid" dependencies. However, the PR did not build, because ir proposes we jump forward 3 major releases forward, and there were API changes between that version and what we use. In consequence, it's necessary to make changes in how we import and use this dependency. Here are a few highlights: - Switch to use the "@ag-grid-community/*" modules - Theme ag-theme-balham.css: both light and dark themes are now bundled in the same file - ColumnApi has been deprecated, then removed. its features have been moved to the GridApi, which we now need to use - need to explicitly import and register infinte-row-model module, before we can use it to scroll in our tables: @ag-grid-community/infinite-row-model - no longer possible to set data source using "api.setDatasource([...])". now using "api.setGridOption('datasource', [...])" instead also: Fix formatting issue reported by yarn format:check ag-grid table: disable column sort, filter buttons Defaults have probably changed since the last major version we used, so we now need to explicitly disable column sorting and suppress the column filter button, when configuring the table columns, to keep the same features we had before. Temporarily disable most tests in table-components.test.tsx The upgrade of ag-grid had made unavailable some of the APIs that are used in this test file. e.g. it's no longer possible to create a column using "new Column([...])". Same for creating a GridApi object. I'm not sure if we can work-around these new limitations or if we will need to redesign these tests. Co-authored-by: Marc Dumais <[email protected]> Signed-off-by: Marc Dumais <[email protected]>
Align to typescript 4.9.5 update @fortawesome/react-fontawesome to 0.2.x to remove ReactDom warning Add license check exclusion for [email protected]: Also took the opportunity to remove a previous exclusion entry that's no longer relevant Pin 3PP rimraf to ^5.0.0: Version ^6.0.0 required node 20, which we have not yet adopted in this repository Signed-off-by: Marc Dumais <[email protected]>
d645971
to
14682d2
Compare
New version pushed that contains 2 commits |
14682d2
to
46ec329
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks for the upgrade of ag-grid.
Thanks for the review! |
ag-grid upgrade
Adapt to later ag-grid version
Dependabot opened a PR to suggest we update our "ag-grid" dependencies.
However, the PR did not build, because ir proposes we jump forward 3
major releases forward, and there were API changes between that version
and what we use.
In consequence, it's necessary to make changes in how we import and use this
dependency. Here are a few highlights:
the same file
to the GridApi, which we now need to use
use it to scroll in our tables: @ag-grid-community/infinite-row-model
using "api.setGridOption('datasource', [...])" instead
TODO:
the tests inAt the suggestion of @MatthewKhouzam I have disabled the failing tests for the moment. See related commit for more details. update: follow-up issue, to fix the tests: Re-enable table-components.test.tsx tests #1100packages/react-components/src/components/__tests__/table-renderer-components.test.tsx
do not pass anymore - they were relying on having access to thegridApi/columnApi
objects, which has been restricted in later versions ofag-grid
.the vscode trace viewer pulls the ag-grid packages. At first look it seems to be just to use the .css files [1], but to be safe it's probably better to test the vscode extension using this PR's version of the "react components" package.Seems to work ok: try this draft PR locally: Upgrade traceviewer* packages to v0.2.6, that include upgraded ag-grid vscode-trace-extension#262 . Note: it assumes repotheia-trace-extension
sits at the same level is itself and has been build using this PR here.[1]:
if that's the case, I think the packages can be replaced by the much smallerConfirmed that vscode-trace-extension only requires .css files, provided now by npm package@ag-grid-community/styles
@ag-grid-community/styles
.yarn upgrade
yarn audit before:
yarn audit after: