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

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

Merged
merged 1 commit into from Jan 29, 2024

Conversation

evertharmeling
Copy link
Contributor

@evertharmeling evertharmeling commented Jan 12, 2024

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

@smnandre
Copy link
Collaborator

Do we add the "module" thing back now.. or in a distinct release to play safe ? cc @weaverryan

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for getting this! Are the tests using chart.js 4? Irrc, they use what's in yarn.lock

src/Chartjs/assets/package.json Show resolved Hide resolved
@evertharmeling
Copy link
Contributor Author

evertharmeling commented Jan 17, 2024

Thanks for getting this! Are the tests using chart.js 4? Irrc, they use what's in yarn.lock

Not sure what's used in the github flow, but locally I tested it installing v3.4.1 and v4.0 manually and the suite passed.

Or should I include this 'hack' again to be sure? But probably we need a more robust solution for supporting multiple versions in the testsuite, as Svelte is atm supporting 2 versions and probably when Turbo 8 is released we need to continue supporting Turbo 7 at the same time. So it's something we'll come across more often.
See #1417

weaverryan added a commit that referenced this pull request Jan 29, 2024
…ndencies per component (evertharmeling)

This PR was squashed before being merged into the 2.x branch.

Discussion
----------

Run tests for all supported versions defined for peerDependencies per component

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes <!-- please update src/**/CHANGELOG.md files -->
| Issues        | Relates to #1389
| License       | MIT

Refactored the `run-vitest-all.sh` script to loop over every component/workspace and check if there are `peerDependencies` who defined support for multiple versions (like https://github.com/symfony/ux/blob/2.x/src/Svelte/assets/package.json#L23). The script then runs the tests for every defined version for that library in that workspace.

The greatest benefit of this change is that this needs no extra administration in keeping a `matrix` value up-to-date in the GitHub workflow for every library in a workspace that supports more than 1 version as it checks every `package.json` automatically.

Tell me what you think about it! Cheers!

- https://github.com/symfony/ux/actions/runs/7646573943/job/20835661388?pr=1417#step:6:200 -> `svelte@3.59.2` is installed
- https://github.com/symfony/ux/actions/runs/7646573943/job/20835661388?pr=1417#step:6:259 -> `svelte@4.2.9` is installed

Commits
-------

16fa514 Run tests for all supported versions defined for peerDependencies per component
@weaverryan
Copy link
Member

Thanks Evert!

@weaverryan weaverryan merged commit 72eaa1a into symfony:2.x Jan 29, 2024
34 checks passed
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.

[ChartJs] Add support for chart.js:^4.0
3 participants