Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(bazel): disable treeshaking when generating FESM and UMD bundles #32069
fix(bazel): disable treeshaking when generating FESM and UMD bundles #32069
Changes from all commits
78205a6
12ace10
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: with this approach, all methods from tslib will be inlined.
I know that @alexeagle had in mind to put
tslib
as a peerDepedency instead of it being a direct dependency, which eventually would make tslib helpers as externals for UMD bundles as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could tell rollup that tslib is external right? I think that's the mechanism that would prevent it being bundled in; then we'd need to fix any package.json files to warn users using the peerDep mechanism. Do you plan to do that as a follow-up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we could for umd, we are explicitly stating the we should include tslib
angular/packages/bazel/src/ng_package/ng_package.bzl
Line 319 in 12ace10
I would like to follow this up and take on TOOL-836, though would like to confirm that @IgorMinar is onboard, if my memory serves me well, he had some concerns not to include tslib as a direct dependency in version 6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just came across this.
tslib
is now a peer dependency for all Angular packages. Though we still bundle tslib into the UMD bundles. This seems to cause unnecessary bloat when multiple UMD bundles are loaded.Ideally we'd just not bundle
tslib
into it, and require consumers to load tslib before. I think we don't do this right now because there is no UMD bundle fortslib
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth looking into this again. Potentially we could help
tslib
with providing an UMD version so that we can move forward with this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with not inlining tslib is that it be a breaking change because we will breaks users who uses umds.
See also: #32167 (review)
What is the problem you are seeing? Or is it just a size matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just a matter of size. It makes sense that we continue bundling it for backwards compatibility. Though, I'm wondering if we could/should revisit this in the future. It seems not ideal to include
tslib
into the bundles multiple times, given thatrxjs
is external too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally the fact that tree-shaking had to be disabled here, doesn't make it better. As you already stated, it means that the whole
tslib
library is included even if only parts are needed.