[ChartJs] chart.js v4 support #610

evertharmeling · 2022-12-14T13:27:31Z

Q	A
Bug fix?	no
New feature?	yes
Tickets	Fix #607
License	MIT

Current status:

Separate controllers per version (3 and 4)
- We could also add specific support for the 3.9 version, see [ChartJS] Fix failing build with ChartJS >= 3.9.0 #427
Added separate test controllers per version
Added support for running low/high dependency tests via GH workflow

Todo

Fix importing chart.js in the chart_v4_controller.ts and fix the test-suite
Test specific low/high dependency dir/paths
- on low-deps (chart.js:^3.4.1); only test chart_v3_controller.test.ts and on high-deps (chart.js:^4.0); only test chart_v4_controller.test.ts. Otherwise v3 deps will error on v4 controller and vice versa.
Files in dist directory should also be updated? Or is that part of the Webpack loader magic :)
Cleanup/reuse (DRY) code in the test controllers?

simonsolutions · 2022-12-14T14:50:06Z

Does this line also has to be changed?

ux/src/Chartjs/assets/package.json

Line 23 in 2363995

"chart.js": "^3.4.1 <3.9",

evertharmeling · 2022-12-14T15:03:55Z

Good catch! Guess that could even be set to the latest version only ("chart.js": "^4.0") right?

simonsolutions · 2022-12-14T15:14:53Z

Good catch! Guess that could even be set to the latest version only ("chart.js": "^4.0") right?

I guess so too, in development the latest version of chart.js should be used, because that is the only where the maintainer of the chart.js repository are giving support for.

evertharmeling · 2022-12-14T15:31:57Z

😕 the fails don't seem directly related to the changes...?

evertharmeling · 2022-12-15T09:16:48Z

Oh wait it looks like it is related, the way 'chart.js/auto' is exposed (used in https://github.com/symfony/ux/blob/2.x/src/Chartjs/assets/src/controller.ts#L13) changed.

Error:

https://github.com/symfony/ux/actions/runs/3696243151/jobs/6259682636

Change

However not sure how to fix supporting both versions... (my js/ts knowledge lacks 😅)

weaverryan · 2022-12-15T15:45:25Z

Oh yes, I forgot about that! So, indeed, the way the chart is imported changed in 3.9 and we couldn’t figure out how to get it working :/. #427. So, you were showing the difference between 3.9 and 4.0, but even 3.8 to 3.9 changed.

Anyways, we can’t drop support for Chart.js 3 without triggering a new major version of UX. The correct solution probably involves having 2 controllers in the bundle: one for 3.8 and lower and one for 3.9 and above (or maybe one for 3.8 and lower and another for 4.0 and above iv 3.9 and 4.0 are actually different as you mentioned). In theory, I think that would be fairly easy to do.

Then, in the package.json file for the UX package, we would need to expose TWO controllers: one would be enabled and the other disabled by default. One trick, however, is that we need the "name" of each controller to be the same. So, for the new controller (the one that supports Chart.js 4), we would use the name key (example https://github.com/symfony/ux/blob/2.x/src/LiveComponent/assets/package.json#L12) to set the name to the "normal" name - which, iirc, would be symfony--ux-chartjs--chart.

So, if possible, I think this is what we should do :). We MIGHT, once this is done, be able to make some tweak in https://github.com/symfony/stimulus-bridge so that, instead of enabling 1 controller and disabling another controller in controllers.json, we choose which controller to use dynamically based on the version of Chart.js the user has installed. But, let's get the work done here first and then investigate that, which involves some Webpack loader magic :).

evertharmeling · 2022-12-15T19:54:08Z

No worries, thanks for the elaborate explanation. Let me put on my superman suit and try to jump in this twilight zone! 😬 🦸🏼‍♂️

evertharmeling · 2022-12-15T22:01:53Z

Something like this... 😟😰 I'm kinda lost at the import part as the (chart.js) docs:3.8.2 / docs:v3.9.1 / docs:4.0 just refer to import Chart from 'chart.js/auto'; which should work for all versions...

To simplify for now I reverted the exclusion of 3.9, as if you want to use the latest version you could just head over to 4.0
I added a AbstractChartController to keep it a bit DRY

weaverryan

This is excellent work... though I feel like we're still working so that we can ultimately discover the problem / solution for what's different between v3 and v4 :p.

Can you update the tests - they're still importing the old controller.ts file, so we can't see the actual failure. Btw, are you able to test this in a local project? Is the v4 version working?

src/Chartjs/assets/package.json

src/Chartjs/assets/src/chart_v4_controller.ts

simonsolutions · 2022-12-20T10:41:22Z

I upgraded chart.js to the 4.x version with Symfony UX component, and so far no issues and problems with chart rendering.

evertharmeling · 2022-12-20T11:07:34Z

I upgraded chart.js to the 4.x version with Symfony UX component, and so far no issues and problems with chart rendering.

Yeah, got it working too, however the tests in this PR showed an error.

weaverryan · 2022-12-20T16:43:10Z

src/Chartjs/assets/src/chart_v4_controller.ts

+'use strict';
+
+import AbstractChartController from './abstract_controller';
+import Chart from 'chart.js/auto';


I suspect that this is caused by something "odd" with our setup (I believe it's because Chart.js 4 is ESM-only) but to fix the test:

Suggested change

import Chart from 'chart.js/auto';

import Chart from 'chart.js/auto/auto.cjs';

But I'm not sure that's the proper solution... and it seems like, from a user's perspective, chart.js/auto DOES work, and we shouldn't need 2 controllers to solve this problem. Again, I've got the feeling that we're doing something silly...

Well that indeed seemed to fix the problem, and the tests in the PR are still failing because of reason mentioned in the todo-list.

Yeah I got that hunch too, seems like we are only satisfying the test-suite for some reason. But that part goes beyond my current js (test-suite) knowledge unfortunately...

I looked at something like resolve, to dynamically check for the module existence so we could still have a single controller. But couldn't get that to work 😞

Btw, I think that:

A) Our code already works with V4 and we just need to allow it in package.json. I think you and/or other users have already verified this.
B) The actual test failing problem is a problem with our tests - specifically, I think jest is behaving poorly because it doesn't it doesn't play well with modules.

Possible solution is to move from jest to https://vitest.dev/

94noni

Nice one 👍🏻
Moreover as it is the lib showcased in the readme :)

simonsolutions · 2023-03-27T18:42:46Z

Does the version in package.json has to be incremented?

ux/src/Chartjs/assets/package.json

Line 5 in 267a5f7

"version": "1.1.0",

boedah · 2023-07-24T11:05:27Z

@weaverryan I can also confirm v4 of ChartJS works.
However, each time we update/install composer packages, the "chart.js": "^4.2.1" in package.json gets reset to ^3.4.1 (which has to be rolled-back manually each time).

Is there something we can do to get a release for this change?
(besides having to rewrite the tests using vitest, as u mentioned :)

weaverryan · 2023-07-31T20:45:48Z

Is there something we can do to get a release for this change?
(besides having to rewrite the tests using vitest, as u mentioned :)

That's all I know of :p. Though, I'm not sure it's a full rewrite - just the lower plumbing needs to be changed if my research was all correct. The other option is to let some tests fail for chart.js / silence them, which also sucks. Simply put: we can't find a way (other than my proposed idea) to get the tests to pass with chart.js v4, even though we all know that v4 works fine.

pculka · 2023-09-07T12:52:30Z

Any ETA on supporting 4.0+ btw? Will it hit the 6.4LTS release?

This PR was merged into the 2.x branch. Discussion ---------- Migrate from Jest to Vitest | Q | A | ------------- | --- | Bug fix? | no | New feature? | no  | Tickets | #1018 #610 | License | MIT This PR is a draft to test the feasibility of migrating from [jest ](https://jestjs.io/) to [vitest](https://vitest.dev/) Jest doesn't support ESM out of the box, which makes it difficult to test some libraries like Chart.js and Svelte v4. Vitest supports typescript and ESM by default and is compatible with Jest API, it'll help us test those packages and maybe others in the future #### Infos: There is a global config file `vitest.config.js` which contains the default settings for every package, when needed it can be extended with a file in the package root (ex: `src\React\assets\vitest.config.js`) I replaced the `yarn test` script with `vitest src --run` so it's run globally and the CI doesn't stop at the first error from a package, it still can be run for every/individual packages with `yarn workspaces run vitest --run` / `yarn workspace "`@symfony`/ux-live-component" run vitest --run` Libraries like fetch-mock-jest and jest-canvas-mock need to be replaced The `require_context_poylfill.ts` file used by ux-react, ux-vue and ux-svelte needs to be modified as it uses a dynamic `require`, replacing it with an `import` doesn't work as it is asynchronous #### Resources: - https://vitest.dev/guide/migration.html - https://srivastavaankita080.medium.com/jest-vitest-migration-79f4735dd5d0 ### TODO: - Packages: - [x] `@symfony`/ux-autocomplete `0/14` - [x] replace fetch-mock-jest ? - [x] `@symfony`/ux-chartjs `0/4` - [x] fix failing tests - [x] replace jest-canvas-mock ? - [x] `@symfony`/ux-cropperjs `1/1` - [x] `@symfony`/ux-dropzone `3/3` - [x] `@symfony`/ux-lazy-image `1/1` - [x] `@symfony`/ux-live-component `217/217` 💯 - [x] `Error: Uncaught [ReferenceError: Node is not defined]` Just a warning but the test suite pass - [x] `@symfony`/ux-notify `1/1` - [x] `@symfony`/ux-react `0/4` - [x] install `@vitejs`/plugin-react - [x] migrate `require_context_poylfill.ts` - [x] `@symfony`/stimulus-bundle `1/1` - [x] `Error: Uncaught [ReferenceError: Node is not defined]` Just a warning but the test suite pass - [x] `@symfony`/ux-svelte `0/4` - [x] replace svelte-jester with `@sveltejs`/vite-plugin-svelte - [x] migrate `require_context_poylfill.ts` - [x] fix failing tests - [x] `@symfony`/ux-swup `0/5` - [x] `require() of ES Module node_modules/delegate-it/index.js from node_modules/swup/lib/index.js not supported.` - [x] `@symfony`/ux-toggle-password `1/1` - [x] `@symfony`/ux-translator `91/91` - [x] `@symfony`/ux-turbo `0/2` - [x] `TypeError: Cannot read properties of undefined (reading 'prototype')` - [x] `@symfony`/ux-typed `1/1` - [x] `@symfony`/ux-vue `0/5` - [x] replace `@vue`/vue3-jest with `@vitejs`/plugin-vue - [x] migrate `require_context_poylfill.ts` - [x] Remove jest - [x] Cleanup - [ ] Benchmark ? I'll need help to resolve these issues 😁 Commits ------- 76d82de Removing need for jq 7f37e59 vitest.config.js format f953d52 Remove jest config and dependencies de32106 Import vitest when needed instead of adding global types in tsconfig 9322bc7 format 2be4088 re-adding module a33fc1d Fixing final tests f59f4f6 Working around Turbo issue 13c7431 dropping support for very old versions of swup ecba87b Adding script to run all tests, but keep going if a previous one fails 238d25e Stopping Stimulus to avoid side effects after the test cf29587 Replacing require.context polyfill with a simpler implementation ef34031 Swapping in vitest-canvas-mock 2270406 Switching to vitest-fetch-mock 9043da3 Removing no-longer-needed setup file c1db340 Fixing problem where Stimulus sometimes continued after the test 10c5591 Add forgotten package.json from ux-react package e9e16eb Wip trying to migrate from jest to vitest

boedah · 2023-11-15T17:09:52Z

@weaverryan I just saw the fantastic #1202 - but have no idea what is left to be done here.
Maybe just release a new major (?) ux-chartjs which requires Chart.js 4+?

This PR was merged into the 2.x branch. Discussion ---------- [ChartJs] Add support for chart.js:^4.0 | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | Issues | Fix #607 | License | MIT Since #1202 has been merged, which solved the blockage of #610, the support for ChartJs v4 should be as simple as this PR :) and would replace #610. 🤞🏼 on the CI-flow Commits ------- 56d925a [ChartJs] Add support for chart.js:^4.0

evertharmeling · 2024-01-30T18:55:06Z

Fixed by #1389

evertharmeling changed the title ~~Chartjs v4 support~~ [ChartJs] chart.js v4 support Dec 14, 2022

evertharmeling added 3 commits December 15, 2022 22:43

Add support for chart.js:^4.0

f314d25

Update CHANGELOG.md

768507e

Update devDependency for chart.js

7a36a4f

evertharmeling force-pushed the chartjs-v4-support branch from f1a941f to e4fd5dd Compare December 15, 2022 21:45

evertharmeling added 2 commits December 15, 2022 22:54

Exclude v3.9

5cb0ee9

Create controller per version

42fb419

evertharmeling force-pushed the chartjs-v4-support branch from e4fd5dd to 42fb419 Compare December 15, 2022 21:55

weaverryan reviewed Dec 20, 2022

View reviewed changes

src/Chartjs/assets/package.json Outdated Show resolved Hide resolved

src/Chartjs/assets/src/chart_v4_controller.ts Show resolved Hide resolved

Create test controller per version (WIP)

f86ba3b

evertharmeling force-pushed the chartjs-v4-support branch 3 times, most recently from 10aa4f2 to 889fcb9 Compare December 20, 2022 12:48

Add support for 'low/high' dependency testing

2e8def6

evertharmeling force-pushed the chartjs-v4-support branch from 889fcb9 to 2e8def6 Compare December 20, 2022 12:51

weaverryan reviewed Dec 20, 2022

View reviewed changes

Fix v4 import

0799e87

94noni approved these changes Feb 17, 2023

View reviewed changes

weaverryan mentioned this pull request Oct 16, 2023

[Svelte] Svelte v4 support #1018

Merged

ChqThomas mentioned this pull request Oct 18, 2023

Migrate from Jest to Vitest #1202

Merged

33 tasks

evertharmeling mentioned this pull request Jan 12, 2024

[ChartJs] Add support for chart.js:^4.0 #1389

Merged

evertharmeling closed this Jan 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ChartJs] chart.js v4 support #610

[ChartJs] chart.js v4 support #610

evertharmeling commented Dec 14, 2022 •

edited

Loading

simonsolutions commented Dec 14, 2022

evertharmeling commented Dec 14, 2022

simonsolutions commented Dec 14, 2022

evertharmeling commented Dec 14, 2022 •

edited

Loading

evertharmeling commented Dec 15, 2022 •

edited

Loading

weaverryan commented Dec 15, 2022

evertharmeling commented Dec 15, 2022 •

edited

Loading

evertharmeling commented Dec 15, 2022 •

edited

Loading

weaverryan left a comment

simonsolutions commented Dec 20, 2022

evertharmeling commented Dec 20, 2022

weaverryan Dec 20, 2022

evertharmeling Dec 20, 2022

evertharmeling Dec 20, 2022

weaverryan Jan 9, 2023

94noni left a comment

simonsolutions commented Mar 27, 2023

boedah commented Jul 24, 2023

weaverryan commented Jul 31, 2023

pculka commented Sep 7, 2023

boedah commented Nov 15, 2023

evertharmeling commented Jan 30, 2024

	import Chart from 'chart.js/auto';
	import Chart from 'chart.js/auto/auto.cjs';

[ChartJs] chart.js v4 support #610

[ChartJs] chart.js v4 support #610

Conversation

evertharmeling commented Dec 14, 2022 • edited Loading

simonsolutions commented Dec 14, 2022

evertharmeling commented Dec 14, 2022

simonsolutions commented Dec 14, 2022

evertharmeling commented Dec 14, 2022 • edited Loading

evertharmeling commented Dec 15, 2022 • edited Loading

weaverryan commented Dec 15, 2022

evertharmeling commented Dec 15, 2022 • edited Loading

evertharmeling commented Dec 15, 2022 • edited Loading

weaverryan left a comment

Choose a reason for hiding this comment

simonsolutions commented Dec 20, 2022

evertharmeling commented Dec 20, 2022

weaverryan Dec 20, 2022

Choose a reason for hiding this comment

evertharmeling Dec 20, 2022

Choose a reason for hiding this comment

evertharmeling Dec 20, 2022

Choose a reason for hiding this comment

weaverryan Jan 9, 2023

Choose a reason for hiding this comment

94noni left a comment

Choose a reason for hiding this comment

simonsolutions commented Mar 27, 2023

boedah commented Jul 24, 2023

weaverryan commented Jul 31, 2023

pculka commented Sep 7, 2023

boedah commented Nov 15, 2023

evertharmeling commented Jan 30, 2024

evertharmeling commented Dec 14, 2022 •

edited

Loading

evertharmeling commented Dec 14, 2022 •

edited

Loading

evertharmeling commented Dec 15, 2022 •

edited

Loading

evertharmeling commented Dec 15, 2022 •

edited

Loading

evertharmeling commented Dec 15, 2022 •

edited

Loading