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

(fix) bundle-size: reuse ts helper functions from tslib instead of in… #13350

Merged
merged 1 commit into from
Feb 22, 2024

Conversation

xcariba
Copy link
Contributor

@xcariba xcariba commented Feb 6, 2024

What it does

Fixes #13345

Reuses ts helper functions from tslib instead of inlining

How to test

Build master, check its size, build this branch, size will be smaller

Follow-ups

none

Review checklist

Reminder for reviewers

@xcariba xcariba force-pushed the reuse-ts-helper-functions branch 4 times, most recently from f8dab9b to b0ffcd8 Compare February 8, 2024 10:41
Copy link
Member

@msujew msujew 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 the contribution. The CLI actually also generates code that depends on tslib. Any project that just pulls @theia/cli without pulling @theia/core will break with this change. So you need to add the dependency there as well.

@xcariba xcariba force-pushed the reuse-ts-helper-functions branch from b0ffcd8 to c245f6d Compare February 8, 2024 14:38
@msujew
Copy link
Member

msujew commented Feb 8, 2024

@xcariba FYI, it's not only @theia/cli, but all packages under dev-packages which need this as well. In every one of them is a require('tslib') call in the generated code now.

Alternatively, we could think about only using this in a few selected packages that are guaranteed to get pulled like @theia/core and @theia/application-package.

@sdirix
Copy link
Member

sdirix commented Feb 8, 2024

Alternatively, we could think about only using this in a few selected packages that are guaranteed to get pulled like @theia/core and @theia/application-package.

Personally I would prefer having a clean dependency in each package. The additional effort is really minimal, because if we need to update all versions then a simple search-replace will do.

I'm not sure whether it's of practical relevance but not declaring the dependency in each package could lead to weird errors. For example a "random" tslib version being resolved for the ones which don't declare, potentially leading to errors, especially in downstream projects.

@msujew
Copy link
Member

msujew commented Feb 8, 2024

@sdirix Yeah, I can see that. I think the maintenance overhead is negligible and it probably makes sense to prevent weird dependency resolving errors.

@Alexander-Taran
Copy link
Contributor

Alexander-Taran commented Feb 8, 2024

@sdirix @msujew
having it in dependencies is relevant only for cases when a package will get bundled or executed outside of scope where @theia/cli is pulled.

In case of /packages/ we want to ensure that tslib is present in node_modules at the time of bunlding the app.
Do dev-packages have a story being pulled on their own?
#13345 (comment)

@sdirix
Copy link
Member

sdirix commented Feb 8, 2024

Hi @Alexander-Taran,

The error case described above is also relevant for the dev-packages. Dev packages like @theia/application-manager and @theia/application-package are consumed by @theia/cli and are therefore part of the node_modules of all downstream projects.

If only @theia/cli has a dependency to tslib, then only for the @theia/cli code itself it's guaranteed to consume the correct version of tslib. Once the code execution enters @theia/application-manager an incompatible tslib coming in from the downstream project might be consumed leading to errors.

Even if that would not be a use case: All published packages should have the correct set of dependencies as we can never predict what consumers will do with them.

@Alexander-Taran
Copy link
Contributor

@sdirix that's exactly what I meant in my comment to my original PR. That we (our team) don't know weather dev-packages need this dependency. And if so - which ones do.

@msujew
Copy link
Member

msujew commented Feb 8, 2024

@Alexander-Taran Once you start setting the importHelper property to true, every package that is affected by this setting should have the tslib dependency. Otherwise, one might into dependency resolution errors (either during bundling or runtime).

@xcariba xcariba force-pushed the reuse-ts-helper-functions branch from c245f6d to 093cd01 Compare February 12, 2024 08:54
@Alexander-Taran
Copy link
Contributor

@msujew @sdirix we've updated dev-packages also. (-:

@Alexander-Taran
Copy link
Contributor

💤

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Looks good to me. Can you rebase so we can merge this?

@Alexander-Taran
Copy link
Contributor

Looks good to me. Can you rebase so we can merge this?

@xcariba is in process

…lining

Signed-off-by: Alexander Kozinko <xcariba@gmail.com>
This was referenced Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Shrink bundle by 500Kb with "importHelpers": true typescript setting
5 participants