Update echarts dep to v6 #2250
Update echarts dep to v6 #2250
Conversation
🦋 Changeset detectedLatest commit: 098b6b0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Summary of ChangesHello @hemi-siemens, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request updates the core Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the echarts dependency to version 6. This is a great step to keep the project up-to-date. However, there are a couple of important points to consider. Firstly, updating a peer dependency to a new major version is a breaking change for the consumers of this package. To adhere to semantic versioning, the package's own version should be bumped to a new major, for example, 4.0.0 instead of 3.0.1. Secondly, I've noticed that the end-to-end tests might be running against an older, hardcoded version of echarts loaded from a CDN in the test HTML files (e.g., packages/echarts/tests/basic.html). It's critical to ensure these tests run against the new echarts v6 to properly validate this change.
| "directory": "packages/echarts" | ||
| }, | ||
| "version": "3.0.0", | ||
| "version": "3.0.1", |
There was a problem hiding this comment.
Updating a peer dependency to a new major version (like echarts from v5 to v6) is a breaking change for the consumers of this package. According to semantic versioning (SemVer) conventions, this package should also receive a major version bump to signal this change. Since you mentioned you're new to versioning, the idea is to alert developers using @siemens/ix-echarts that they need to handle potential breaking changes from echarts v6. Therefore, the version should be updated to 4.0.0.
| "version": "3.0.1", | |
| "version": "4.0.0", |
hemi-siemens
force-pushed
the
update-dep-echarts-v6
branch
from
fc18629 to
203caa0
Compare
There was a problem hiding this comment.
Thanks for contribution.
I have some comments please check the review.
In general you added some windows screenshots please read the https://github.com/siemens/ix/blob/main/CONTRIBUTING.md#visual-regression-testing
visual-tests needs to be execute only within a linux based docker container e.g mcr.microsoft.com/playwright:v1.54.1-noble . Please create linux images from the container and remove the windows images
| "directory": "packages/echarts" | ||
| }, | ||
| "version": "3.0.0", | ||
| "version": "3.0.1", |
There was a problem hiding this comment.
Please do not update version inside package.json this will be done via changeset
There was a problem hiding this comment.
Updating echarts to V6 is a breaking change imo. Does it not work to support v5 and v6 here to avoid a major breaking change?
Hows about having v5 or v6 as a peerDependency?
Execute tests for v5 and Execute tests for v6 should verify if both versions are compatible with the latest theming file
|
2 participants
💡 What is the current behavior?
Currently dependent on echarts ^5.5.1
GitHub Issue Number: None
🆕 What is the new behavior?
Updated dependency to echarts ^6.0.0
https://echarts.apache.org/handbook/en/basics/release-note/v6-feature/#
🏁 Checklist
A pull request can only be merged if all of these conditions are met (where applicable):
pnpm test)pnpm lint)pnpm build, changes pushed)👨💻 Help & support
First contribution here, I am unsure of the versioning semantics and all the tooling.