Relax tslib requirement specifier

mrginglymus commented 2 weeks ago

Brief Information

This pull request is in the type of:

[ ] bug fixing
[ ] new feature
[x] others

What does this PR do?

Relax dependency specifier of tslib

Fixed issues

Details

Before: What was the problem?

I was testing my library with vite v6.beta, and found that tslib@2.3.0 did not work; later version of tslib did work.

After: How does it behave after the fixing?

echarts now allows users to pick their own version of tslib - so a. it now works with vite v6 and b. users aren't unecessarily shipping two copies of tslib

Document Info

One of the following should be checked.

[x] This PR doesn't relate to document changes
[ ] The document should be updated later
[ ] The document changes have been made in apache/echarts-doc#xxx

Misc

ZRender Changes

[x] This PR depends on ZRender changes (ecomfe/zrender#1097).

Related test cases or examples to use the new APIs

N.A.

Others

Merging options

[ ] Please squash the commits into a single one when merging.

Other information

echarts-bot[bot] commented 2 weeks ago

Thanks for your contribution! The community will review it ASAP. In the meanwhile, please checkout the coding standard and Wiki about How to make a pull request.

github-actions[bot] commented 2 weeks ago

The changes brought by this PR can be previewed at: https://echarts.apache.org/examples/editor?version=PR-20485@c939973

echarts-bot[bot] commented 1 week ago

Congratulations! Your PR has been merged. Thanks for your contribution! 👍

100pah commented 1 week ago

Curious about why tslib 2.3.0 doesn't work.

I can not reproduce it with vite@6.0.0-beta.9. Could I have more info to reproduce it? @mrginglymus

What I've done is:

npm i -D vite@6.0.0-beta.9
npm i echarts
touch index.html

            <!DOCTYPE html>
            <html>
                <head>
                    <meta charset="utf-8">
                    <meta name="viewport" content="width=device-width, initial-scale=1" />
                    <script src="./node_modules/echarts/dist/echarts.js"></script>
                </head>
                <body>
                    <p>good</p>
                    <div id="main" style="height: 500px; "></div>
                    <script>
                        var chart = echarts.init(document.getElementById('main'));
                        chart.setOption({
                            angleAxis: {
                                type: 'category',
                                data: ['周一', '周二', '周三', '周四', '周五', '周六', '周日'],
                            },
                            tooltip: {},
                            radiusAxis: {},
                            polar: { center: ['40%', '50%'] },
                            series: [{
                                type: 'bar',
                                data: [1, 2, '-', 3, 0, 5, 7],
                                coordinateSystem: 'polar'
                            }]
                        });
                        chart.on('click', function (params) {
                            console.log(params);
                        });
                    </script>
                </body>
            </html>

npx vite

And the chart displayed normally.

@Ovilia @plainheart

Does this modification lead to breaking change? Or should we do full test against different version of tslib? or check every breaking changes of tslib since 2.3.0 to latest? Or should we make this change in a patch version if it might introduce breaking or error?

mrginglymus commented 1 week ago

Honestly I didn't spend too much time on working out the why, only the what.

The error as spotted was this:

I will see if I can dig into what was wrong with vite here, as it may be worth an issue on their end as a regression.

Aside from compatibility in whatever obscure set of conditions I've discovered, the more significant benefit of this change is allowing users to ship only one copy of tslib; I currently use tslib@^2.5.0 to support decorators and whilst the size of tslib is insignificant compared to the echarts stack, it still bugs me to be shipping two copies.

100pah commented 1 week ago

Some memos:

Found that at present echarts only uses __extends function of tslib. (by searching from "tslib in echarts/lib.).
About bundle size: After tree shaking (tested in vite@6.0.0-beta.9, tsc -b && vite build), only used functions of tslib will be included in bundle. That is only __extends (and extendStatics, which is called by __extends internally) is included. So the bundle size is probably not a concern till now.
Compatibility if changing to "tslib": "^x.x.x": Checked the commit log of __extends on git, there's no breaking. Thus this change does not break echarts.

Use different versions of tslib in one JS context: (tested in vite@6.0.0-beta.9, tsc -b && vite build) , the built bundle is like this :

// At the top scope of the bundle file dist/assets/index-JsGP8STl.js
// Two tslib are included but the name are modified to avoid conflict.
var extendStatics$1 = function(d, b) { /* ... */ };
function __extends$1(d, b) { /* ... */ }
/* ... */
var extendStatics = function(d, b) { /* ... */ };
function __extends(d, b) { /* ... */ }

apache / echarts