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.

Sign up for GitHub

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

Jump to bottom

Relax tslib requirement specifier #20485

Merged
merged 1 commit into from
Nov 8, 2024
Merged

Relax tslib requirement specifier #20485

merged 1 commit into from
Nov 8, 2024

Conversation

mrginglymus
Copy link
Contributor

@mrginglymus mrginglymus commented Nov 7, 2024 •
edited
Loading

Brief Information

This pull request is in the type of:

  • bug fixing
  • new feature
  • 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 [email protected] 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.

  • 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

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 ECharts Bot
Copy link

echarts-bot bot commented Nov 7, 2024

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.

@mrginglymus mrginglymus mentioned this pull request Nov 7, 2024
Merged
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 8, 2024

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

@Ovilia Ovilia merged commit 18f8ea3 into apache:master Nov 8, 2024
2 checks passed
@echarts-bot ECharts Bot
Copy link

echarts-bot bot commented Nov 8, 2024

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

@mrginglymus mrginglymus deleted the relax-tslib branch November 8, 2024 08:12
@100pah
Copy link
Member

100pah commented Nov 12, 2024

Curious about why tslib 2.3.0 doesn't work.

I can not reproduce it with [email protected].
Could I have more info to reproduce it? @mrginglymus

What I've done is:

npm i -D [email protected]
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
Copy link
Contributor Author

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

The error as spotted was this:
image

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.

@Ovilia Ovilia mentioned this pull request Nov 13, 2024
Merged
8 tasks
@100pah
Copy link
Member

100pah commented Nov 13, 2024 •
edited
Loading

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 [email protected], 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 [email protected], 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) { /* ... */ }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ovilia Ovilia Ovilia approved these changes

Assignees
No one assigned
Labels
PR: doc unchanged PR: first-time contributor size/XS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mrginglymus @100pah @Ovilia