Skip to content
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

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

Merged
merged 1 commit into from Nov 9, 2023

Conversation

weaverryan
Copy link
Member

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 [ChartJs] Replace chart.js/auto import with Chart.register calls #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!

@weaverryan weaverryan merged commit d4e3f0a into symfony:2.x Nov 9, 2023
34 checks passed
@weaverryan weaverryan deleted the revert-chartjs-module branch November 9, 2023 15:29
weaverryan added a commit that referenced this pull request Nov 11, 2023
This PR was squashed before being merged into the 2.x branch.

Discussion
----------

Remove "type": "module" from all package.json files

Since #1264, another person could not build its assets with Encore and a babel plugin.

So for now we decided to revert the "type: module" declaration.

### Revert the "type: module"

Remove this "type" line from all package.json files and revert replacing "main:" with "module:"

Before
```json
   "type": "module",
   "module": "dist/foo.js",
```

After
```json
   "main": "dist/foo.js",
```

### Fix Svelte ESM bug with vite/vitest

```
We recommend converting your config to ESM by either:

    adding "type": "module" to the nearest package.json
    renaming vite.config.js/vite.config.ts to vite.config.mjs/vite.config.mts
```
(https://vitejs.dev/guide/troubleshooting.html#config)

So i used the second one.... and it seems to work

Commits
-------

919ade7 Remove "type": "module" from all package.json files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Chart.js] Build fails with latest 2.13.0 release
1 participant