fix(ag-grid): change theme to function and add charts export path #3816
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: 9029ff6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
🐞 bug
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression introduced in AG-Grid 34.3.1 that causes "object is not extensible" errors in Vite dev mode. The fix changes the theme configuration to use lazy evaluation via functions () => Theme instead of direct Theme values, ensuring proper timing with AG-Grid's updated dependency injection system. Additionally, it introduces a new dedicated ./charts export path for AG Charts components.
Key Changes:
- Theme API converted from
Themeto() => Themefor lazy evaluation - New
./chartsexport path added forAgChartsandAgChartsEnterpriseModule - Cookbook examples updated to use new chart exports and demonstrate theme function API
- Dependencies updated: added
ag-charts-reactand peer dependencies for React 18/19
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
packages/modules/ag-grid/src/AgGridConfigurator.interface.ts |
Changes theme type from Theme to () => Theme in config interface |
packages/modules/ag-grid/src/AgGridConfigurator.ts |
Updates theme storage and setTheme() to wrap values in functions, imports createTheme from local themes module |
packages/modules/ag-grid/src/AgGridProvider.ts |
Updates theme getter and initialization to call theme function with ?.() |
packages/modules/ag-grid/src/module.ts |
Wraps theme initialization in function for lazy evaluation |
packages/react/ag-grid/src/charts.ts |
New file exporting AgChartsEnterpriseModule and AgCharts from dedicated path |
packages/react/ag-grid/src/enterprise.ts |
Adds AgCharts export for backward compatibility with deprecation note |
packages/react/ag-grid/package.json |
Adds ./charts export path, ag-charts-react dependency, and React peer dependencies |
cookbooks/app-react-ag-grid/src/config.ts |
Adds theme customization example using setTheme() callback |
cookbooks/app-react-charts/src/pages/ag-charts/*.tsx |
Updates chart imports to use new @equinor/fusion-framework-react-ag-grid/charts path |
cookbooks/app-react-charts/package.json |
Removes direct ag-charts dependencies, adds framework ag-grid package |
.changeset/*.md |
Changesets documenting the theme function change, charts export addition, and cookbook updates |
pnpm-lock.yaml |
Dependency resolution updates including ag-grid-react now using React 18 instead of 19 |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Collaborator
Author
|
@eikeland and @asbjornhaland - i have assigned the review for @AndrejNikolicEq, but if he still sick tomorrow, please follow up this PR. Note well you could have a look anyway, more 👀 = more ASAP Zulu release - prevents anyone with AG Grid to render their app dev-mode |
BREAKING CHANGE: theme configuration now accepts a function () => Theme instead of Theme directly Fixes #747
Add new ./charts export for AG Charts React components and enterprise module. Includes AgCharts and AgChartsEnterpriseModule exports. Fixes #747
Add test theme override demonstrating theme function API with custom parameters.
Migrate chart components to use @equinor/fusion-framework-react-ag-grid/charts instead of direct ag-charts-react imports. Fixes #747
- Bump '@types/node' from 24.9.2 to 24.10.1 across various packages. - Update 'ag-charts-react' and related dependencies to version 12.3.1. - Adjust 'react' and 'react-dom' versions to 18.3.1. - Upgrade 'rollup' from 4.52.5 to 4.53.3. - Remove deprecated esbuild versions and clean up unused packages. This update ensures compatibility with the latest versions and improves overall project stability. Signed-off-by: Odin Thomas Rochmann <[email protected]>
Co-authored-by: Copilot <[email protected]>
Eliminate the unused import of createTheme from AgGridProvider.ts to clean up the codebase and improve readability. Signed-off-by: Odin Thomas Rochmann <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
asbjornhaland
requested changes
Dec 4, 2025
.changeset/fusion-framework-module-msal_acquire-token-request-default.md
Outdated
Show resolved
Hide resolved
Change exports in charts.ts to use a wildcard export for ag-charts-react, and remove AgCharts export from enterprise.ts to streamline module usage. This enhances clarity and maintains compatibility with the latest ag-charts updates. Signed-off-by: Odin Thomas Rochmann <[email protected]>
Contributor
Comment on lines
32
to
38
| setTheme(valueOrCallback: Theme | ((baseTheme: Theme) => Theme) | null): void { | ||
| if (typeof valueOrCallback === 'function') { | ||
| this.#theme = valueOrCallback(this.#theme ?? createTheme()); | ||
| this._set('theme', async () => () => valueOrCallback(this.#theme())); | ||
| } else { | ||
| this.#theme = valueOrCallback ?? undefined; | ||
| this._set('theme', async () => () => valueOrCallback as Theme); | ||
| } | ||
| } |
There was a problem hiding this comment.
The setTheme method updates the config via _set() but doesn't update the #theme field. This means subsequent calls to setTheme with a callback will use the old theme function, not the newly set one.
The method should also update #theme:
setTheme(valueOrCallback: Theme | ((baseTheme: Theme) => Theme) | null): void {
if (typeof valueOrCallback === 'function') {
this.#theme = () => valueOrCallback(this.#theme());
this._set('theme', async () => this.#theme);
} else {
this.#theme = () => (valueOrCallback ?? createTheme());
this._set('theme', async () => this.#theme);
}
}| this._set('theme', async () => () => valueOrCallback(this.#theme())); | ||
| } else { | ||
| this.#theme = valueOrCallback ?? undefined; | ||
| this._set('theme', async () => () => valueOrCallback as Theme); |
There was a problem hiding this comment.
When valueOrCallback is null, this code casts it to Theme and wraps it in a function. This will result in () => null, which will cause issues when the theme is used.
The handling of null should either set theme to undefined or to a default theme function:
this._set('theme', async () => valueOrCallback ? () => valueOrCallback as Theme : () => createTheme());
asbjornhaland
approved these changes
Dec 4, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why
Why is this change needed?
This PR fixes a regression introduced by PR #3639 (AG-Grid 34.3.1 upgrade) that causes an "object is not extensible" error in Vite dev mode. The issue occurs because AG-Grid 34.3.1 changed its internal dependency injection system, making theme injection timing-sensitive. In Vite dev mode with split ESM chunks,
provideGlobalGridOptions({ theme })now happens after AG-Grid has already created and frozen its internal theme instance, causing the error when AG-Grid tries to add_paramsCache.What is the current behavior?
Themevalue that is evaluated immediately./enterpriseexport pathWhat is the new behavior?
() => Themefor lazy evaluation./chartsexport path for AG Charts components (AgChartsandAgChartsEnterpriseModule)./enterprisefor backward compatibility (will be removed in future minor release)Does this PR introduce a breaking change?
No. While the theme API changes from
Themeto() => Theme, this is marked as a minor version bump. The changeset includes migration instructions showing how to update from direct theme values to theme functions. After upgrading, everything will work again without any code changes on the consumer side (the lazy evaluation handles both function and direct value patterns internally). Charts exports are maintained in./enterprisefor backward compatibility.Additional context
./chartsexport pathRelated issues
closes: https://github.com/equinor/fusion/issues/747
How to Review This PR
Understanding the Problem
This regression was introduced by PR #3639 (AG-Grid 34.3.1 upgrade). AG-Grid v34 introduced several enhancements to its internal dependency injection system that affect theme injection:
ThemeImpl) are now initialized more aggressively during early grid core setup, with stricter sealing of objects to prevent runtime mutations outside the DI container.In Vite dev mode with split ESM chunks,
provideGlobalGridOptions({ theme })now happens after AG-Grid has already created and frozen its internal theme instance, causing the error when AG-Grid tries to add_paramsCache.What to Focus On
Theme function API implementation (
packages/modules/ag-grid/):theme?: () => Themeis consistently used throughout the module chainCharts export path (
packages/react/ag-grid/):./chartsexport path works correctly./enterpriseexportsTesting considerations:
packages/dev-portal/src/config.tsfor reference on how to enable modules using theenable*pattern)cookbooks/app-react-ag-grid/src/config.tsfor the theme override example) to verify the lazy theme function API works correctly and doesn't trigger the "object is not extensible" errorCookbook examples:
Key Files to Review
packages/modules/ag-grid/src/AgGridConfigurator.interface.ts- Interface change totheme?: () => Themepackages/modules/ag-grid/src/AgGridConfigurator.ts- Lazy theme resolution implementationpackages/modules/ag-grid/src/AgGridProvider.ts- Theme getter that calls the functionpackages/react/ag-grid/src/charts.ts- New charts export pathpackages/react/ag-grid/src/enterprise.ts- Backward compatibility exportsChecklist