[ChartJs] Replace `chart.js/auto` import with `Chart.register` calls #1263

smnandre · 2023-11-09T14:00:09Z

Q	A
Bug fix?	yes
New feature?	no
Issues	Fix #1262
License	MIT

Ongoing attempt to fix ChartJS problems

weaverryan · 2023-11-09T14:08:12Z

src/Chartjs/assets/package.json

        }
    },
    "peerDependencies": {
        "@hotwired/stimulus": "^3.0.0",
-        "chart.js": "^3.4.1 <3.9"
+        "chart.js": "^3.9"


For others looking at this, we can absolutely allow v4 right now. But we should do it in a minor release, not a patch release.

weaverryan · 2023-11-09T14:10:50Z

Background info: Chart.js changed some things starting in 3.9, which is why we previously restricted that version (they were arguably BC breaks).

In chart.js 2.13.0, we changed the JS package shipped to type: module. That had an unexpected side effect of no longer loading the chart.js/auto import (even though it loads fine in our test). Tbh, the reasons are a bit of a mystery to me.

Anyway, here is the breakdown of the fix:

A) For Encore users, you will need to upgrade chart.js to ^3.9. You may currently have, for example, 3.8.0, so upgrading to 3.9.* is fine - just a minor update.
B) For AssetMapper users, you will need to importmap:require 'chart.js' (and you no longer need chart.js/auto). Flex may actually do this automatically - I think it will, but we'll need to try it :)

smnandre · 2023-11-09T14:13:03Z

About the typescript error :

I think 4.0 was release the other day and since Chart.js v3 it's been rewritten with Typescript, so you don't need these typedefinitions anymore.

DefinitelyTyped/DefinitelyTyped#63641 (comment)

Just removed the file... let's see

weaverryan · 2023-11-09T14:24:03Z

I'm checking into this failure. What's being imported in the tests is different than what's imported in Encore... which is why our original tests didn't catch the bug.

smnandre · 2023-11-09T15:25:03Z

So we could release this fix, and check all this module/ES/node/2015/6/beta/tango/charlie later ? Or is there a risk ?

This PR was merged into the 2.x branch. Discussion ---------- [Chart.js] Reverting chart.js type: module | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | Issues | Fix #1262 | License | MIT This is complex, and relates to some hard/weird/broken changes that chart.js made in 3.9. Facts: * Changing `type: module` and making no other changes breaks `chart.js` v3 (but v4 works fine) * Changing the import from `chart.js/auto'` to `chart.js` - like #1263 fixes things. However, it breaks the tests in 3.9... because chart.js doesn't have an `exports` key in that version, so our tests / node environment continue to load the `main` key instead of the `module` key. So, this will "put thing back" and get chart.js working again. After this, we should: 1) Re-add `type: module` to chart.js 2) Upgrade `chart.js` to v4 By doing that, other than actually migrating any custom chart.js code from 3 -> 4 (https://www.chartjs.org/docs/latest/migration/v4-migration.html), users won't need to make any other changes. The question about 3 -> 4 is if we can do that in a minor UX version of it needs to be a major. Technically, nothing in `symfony/ux-chart.js` changes... except that it will force you to change a JS dep up a major version. It's a gray area. Cheers! Commits ------- 3fca0ca [Chart.js] Reverting chart.js type: module

weaverryan · 2023-11-09T15:30:30Z

Fixed in #1264. However, we can make this PR the proper PR for the NEXT version, which would include:

Re-adding type: module
Upgrading chart.js to v4

smnandre · 2023-11-09T18:45:00Z

I did not realize Chart.js 4.0 was released 12 months ago.

That could itself be a good argument to push gently users towards an upgrade :)

smnandre · 2024-04-14T22:28:26Z

I rebased and tried to clean this PR i opened a long time ago, but i always end with test failures somewhere...

Could you guys @evertharmeling @simondaigre maybe help me there ?

(cc issue #1695 )

src/Chartjs/assets/src/controller.ts

simondaigre · 2024-04-14T22:44:59Z

src/Chartjs/assets/dist/controller.js


+Chart.register(...registerables);


Suggested change

Chart.register(...registerables);

registerables.forEach(registerable => Chart.register(registerable));

https://stackoverflow.com/questions/75449457/typeerror-chart-registerables-is-not-iterable-cannot-read-property-undefined

I came across this one too, however we still need to verfify registerables != undefined as with this code we would still get the following error on 3.x:

TypeError: Cannot read properties of undefined (reading 'forEach')

And i still think it should not be "undefined" :))

https://github.com/evertharmeling/ux-1263

This seems to backup that it is our testsuite that's messing with us. I've added a console log on the registerables (symfony/ux). And when using [email protected] the registerables variable is NOT undefined.

Ok so its a « module » thing we need to force on the tests… that narrows the problem !

thanks a lot for taking this time it really helps a lot :)

simondaigre · 2024-05-08T18:38:10Z

@smnandre Do you need some help to finish the PR ?

smnandre · 2024-05-08T18:45:44Z

@smnandre Do you need some help to finish the PR ?

That'd be really nice, yes :)

simondaigre · 2024-05-08T22:33:57Z

I also checked and :

chart.js file (non-ESM) for 3.x versions doesn't export registerables : https://cdn.jsdelivr.net/npm/[email protected]/dist/chart.js
Vitest doesn't select the correct version (see Jest Test failed with Angular 14 ans ng2-charts 3.1.0 valor-software/ng2-charts#1421 + How vitest resolves dependency exports vitest-dev/vitest#4233 for more explanations)
I tried to use resolve.alias in vitest.config.js but filenames are not the same between chart.js 3.x and 4.x.

So there is two solutions:

We stick chart.js to 3.x OR 4.x (BC break ?)
Solution provided by @evertharmeling works and as the issue is only related to our Vitest test suite, I think we can use it

smnandre · 2024-05-08T23:31:55Z

@simondaigre thank you so much for taking this time, really 🙇

Let's say we take the solution 2, is there any way to still check 3.x on the CI with no "false failures" ?

simondaigre · 2024-05-08T23:36:35Z

I have no failures on CI with 2/ (for both 3.x and 4.x).

smnandre · 2024-05-09T00:35:43Z

I have no failures on CI with 2/ (for both 3.x and 4.x).

So you'd merge the PR ?

(i updated to take the @evertharmeling suggestion)

simondaigre · 2024-05-09T09:54:30Z

I checked on a real project with both chart.js 3.x and 4.x, it's OK for me: no breaks and it fixes my issue.
(changes to lockfiles seems unrelated to this PR I think).

smnandre · 2024-05-10T20:08:42Z

Well let's merge it and test one last time on the dev branch before the next release ! :)

(@kbond if you can ?)

A massive thank to you guys @simondaigre @evertharmeling !! 🙇

kbond · 2024-05-10T20:54:14Z

Thanks all!

weaverryan reviewed Nov 9, 2023

View reviewed changes

weaverryan approved these changes Nov 9, 2023

View reviewed changes

weaverryan mentioned this pull request Nov 9, 2023

[Chart.js] Reverting chart.js type: module #1264

Merged

smnandre mentioned this pull request Apr 10, 2024

[ChartJS] Using chartjs/auto + importmap is not compatible with date adapters #1695

Closed

smnandre force-pushed the fix/chartjs-encore branch from 63c757d to fd99323 Compare April 14, 2024 21:35

smnandre changed the title ~~Fix Chart.js encore compilation~~ [ChartJs] Replace chart.js/auto import with Chart.register calls Apr 14, 2024

evertharmeling reviewed Apr 15, 2024

View reviewed changes

src/Chartjs/assets/src/controller.ts Outdated Show resolved Hide resolved

simondaigre reviewed Apr 16, 2024

View reviewed changes

smnandre force-pushed the fix/chartjs-encore branch from 0f94046 to 09135eb Compare May 9, 2024 00:29

Fix Chart.js encore compilation

addc1f8

smnandre force-pushed the fix/chartjs-encore branch from 7131c0a to addc1f8 Compare May 9, 2024 00:32

kbond merged commit 796dd58 into symfony:2.x May 10, 2024
35 of 36 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ChartJs] Replace `chart.js/auto` import with `Chart.register` calls #1263

[ChartJs] Replace `chart.js/auto` import with `Chart.register` calls #1263

smnandre commented Nov 9, 2023

weaverryan Nov 9, 2023

weaverryan commented Nov 9, 2023

smnandre commented Nov 9, 2023

weaverryan commented Nov 9, 2023

smnandre commented Nov 9, 2023

weaverryan commented Nov 9, 2023

smnandre commented Nov 9, 2023

smnandre commented Apr 14, 2024 •

edited

Loading

simondaigre Apr 14, 2024

evertharmeling Apr 17, 2024

smnandre Apr 17, 2024

evertharmeling Apr 17, 2024

smnandre Apr 17, 2024

simondaigre commented May 8, 2024

smnandre commented May 8, 2024

simondaigre commented May 8, 2024 •

edited

Loading

smnandre commented May 8, 2024

simondaigre commented May 8, 2024

smnandre commented May 9, 2024

simondaigre commented May 9, 2024 •

edited

Loading

smnandre commented May 10, 2024

kbond commented May 10, 2024

	Chart.register(...registerables);
	registerables.forEach(registerable => Chart.register(registerable));

[ChartJs] Replace chart.js/auto import with Chart.register calls #1263

[ChartJs] Replace chart.js/auto import with Chart.register calls #1263

Conversation

smnandre commented Nov 9, 2023

weaverryan Nov 9, 2023

Choose a reason for hiding this comment

weaverryan commented Nov 9, 2023

smnandre commented Nov 9, 2023

weaverryan commented Nov 9, 2023

smnandre commented Nov 9, 2023

weaverryan commented Nov 9, 2023

smnandre commented Nov 9, 2023

smnandre commented Apr 14, 2024 • edited Loading

simondaigre Apr 14, 2024

Choose a reason for hiding this comment

evertharmeling Apr 17, 2024

Choose a reason for hiding this comment

smnandre Apr 17, 2024

Choose a reason for hiding this comment

evertharmeling Apr 17, 2024

Choose a reason for hiding this comment

smnandre Apr 17, 2024

Choose a reason for hiding this comment

simondaigre commented May 8, 2024

smnandre commented May 8, 2024

simondaigre commented May 8, 2024 • edited Loading

smnandre commented May 8, 2024

simondaigre commented May 8, 2024

smnandre commented May 9, 2024

simondaigre commented May 9, 2024 • edited Loading

smnandre commented May 10, 2024

kbond commented May 10, 2024

[ChartJs] Replace `chart.js/auto` import with `Chart.register` calls #1263

[ChartJs] Replace `chart.js/auto` import with `Chart.register` calls #1263

smnandre commented Apr 14, 2024 •

edited

Loading

simondaigre commented May 8, 2024 •

edited

Loading

simondaigre commented May 9, 2024 •

edited

Loading